Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472283002:
In chrome://settings/fonts, fonts are not alphabetically ordered. (Closed)
DescriptionIn chrome://settings/fonts, fonts are not alphabetically ordered.
NSFontManager does not provide sorted font lists.
So We should sort it manually.
BUG=655594
Committed: https://crrev.com/ac2a392c1984cb75dee0185b723a1f482aa7e77a
Cr-Commit-Position: refs/heads/master@{#430048}
Patch Set 1 #Patch Set 2 : CaseInsensitiveCompare #Patch Set 3 : fix #
Total comments: 2
Patch Set 4 : Change to localizedStandardCompare #Patch Set 5 : Sort by localizedName. #Patch Set 6 : Add comment. #
Total comments: 1
Patch Set 7 : Change sort #
Total comments: 1
Patch Set 8 : nit fix #Messages
Total messages: 24 (11 generated)
corona10@gmail.com changed reviewers: + avi@chromium.org, dbeam@chromium.org
@avi @dbeam PTAL.
Description was changed from ========== In chrome://settings/fonts, fonts are not alphabetically ordered. BUG=655594 ========== to ========== In chrome://settings/fonts, fonts are not alphabetically ordered. BUG=655594 ==========
Description was changed from ========== In chrome://settings/fonts, fonts are not alphabetically ordered. BUG=655594 ========== to ========== In chrome://settings/fonts, fonts are not alphabetically ordered. NSFontManager does not provide sorted font lists. So We should sort it manually. BUG=655594 ==========
On 2016/11/04 02:10:57, dhna wrote: > @avi > @dbeam > PTAL. It works right after PS3. PTAL.
https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... File content/common/font_list_mac.mm (right): https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... content/common/font_list_mac.mm:23: [fontManager localizedNameForFamily:family_name face:nil]; You're sorting based on the family name above, yet what the user ends up seeing is the localized name. How do you know that the sorts will be the same? https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... content/common/font_list_mac.mm:31: return font_list; Perhaps sort the list at this point, with a localized compare (localizedStandardCompare: rather than compare:)?
On 2016/11/04 05:31:53, Avi wrote: > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > File content/common/font_list_mac.mm (right): > > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > content/common/font_list_mac.mm:23: [fontManager > localizedNameForFamily:family_name face:nil]; > You're sorting based on the family name above, yet what the user ends up seeing > is the localized name. How do you know that the sorts will be the same? > > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > content/common/font_list_mac.mm:31: return font_list; > Perhaps sort the list at this point, with a localized compare > (localizedStandardCompare: rather than compare:)? @Avi I change it to localizedStandardCompare. And I will think about how to sort by localized name.
On 2016/11/04 09:21:05, dhna wrote: > On 2016/11/04 05:31:53, Avi wrote: > > > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > > File content/common/font_list_mac.mm (right): > > > > > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > > content/common/font_list_mac.mm:23: [fontManager > > localizedNameForFamily:family_name face:nil]; > > You're sorting based on the family name above, yet what the user ends up > seeing > > is the localized name. How do you know that the sorts will be the same? > > > > > https://codereview.chromium.org/2472283002/diff/40001/content/common/font_lis... > > content/common/font_list_mac.mm:31: return font_list; > > Perhaps sort the list at this point, with a localized compare > > (localizedStandardCompare: rather than compare:)? > @Avi > I change it to localizedStandardCompare. > And I will think about how to sort by localized name. @Avi I fixed it all as your comment. PTAL.
https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... File content/common/font_list_mac.mm (right): https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... content/common/font_list_mac.mm:33: return [localized1 compare:localized2 options:NSCaseInsensitiveSearch]; This isn't a localized sort, but rather a sort on localized names. We need both. The documentation on -[NSString compare:options:] suggests what I suggested, to use -[NSString localizedStandardCompare:]. I think you can simplify this as well. Because your values are NSStrings, this should work: NSArray* sortedFonts = [fonts_dict keysSortedByValueUsingSelector:@selector(localizedStandardCompare:)];
On 2016/11/04 16:33:40, Avi wrote: > https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... > File content/common/font_list_mac.mm (right): > > https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... > content/common/font_list_mac.mm:33: return [localized1 compare:localized2 > options:NSCaseInsensitiveSearch]; > This isn't a localized sort, but rather a sort on localized names. We need both. > The documentation on -[NSString compare:options:] suggests what I suggested, to > use -[NSString localizedStandardCompare:]. > > I think you can simplify this as well. Because your values are NSStrings, this > should work: > > NSArray* sortedFonts = [fonts_dict > keysSortedByValueUsingSelector:@selector(localizedStandardCompare:)]; @Avi Done. Thank you for comment and guide. I'm not familiar with Objective-c I just learn it immediately for fix this issue. :-)
On 2016/11/04 16:40:42, dhna wrote: > On 2016/11/04 16:33:40, Avi wrote: > > > https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... > > File content/common/font_list_mac.mm (right): > > > > > https://codereview.chromium.org/2472283002/diff/100001/content/common/font_li... > > content/common/font_list_mac.mm:33: return [localized1 compare:localized2 > > options:NSCaseInsensitiveSearch]; > > This isn't a localized sort, but rather a sort on localized names. We need > both. > > The documentation on -[NSString compare:options:] suggests what I suggested, > to > > use -[NSString localizedStandardCompare:]. > > > > I think you can simplify this as well. Because your values are NSStrings, this > > should work: > > > > NSArray* sortedFonts = [fonts_dict > > keysSortedByValueUsingSelector:@selector(localizedStandardCompare:)]; > > @Avi > Done. > Thank you for comment and guide. > I'm not familiar with Objective-c I just learn it immediately for fix this > issue. :-) PTAL.
LGTM. There's a small nit that would make it cleaner; feel free to commit with no further review. The one thing I want to keep my eye on is your use of [] on NSDictionary. That _should_ work now that we're on the 10.9 SDK but if it doesn't you might have to fall back to using -setObject:forKey:. :( I know, but we're clawing our way up to modern ObjC slowly. Thank you for learning enough Cocoa to take this on! https://codereview.chromium.org/2472283002/diff/110001/content/common/font_li... File content/common/font_list_mac.mm (right): https://codereview.chromium.org/2472283002/diff/110001/content/common/font_li... content/common/font_list_mac.mm:20: [[[NSMutableDictionary alloc] init] autorelease]; Nit: you can use the convenience constructor: NSMutableDictionary* fonts_dict = [NSMutableDictionary dictionary];
On 2016/11/04 19:59:55, Avi wrote: > LGTM. > > There's a small nit that would make it cleaner; feel free to commit with no > further review. > > The one thing I want to keep my eye on is your use of [] on NSDictionary. That > _should_ work now that we're on the 10.9 SDK but if it doesn't you might have to > fall back to using -setObject:forKey:. > > :( > > I know, but we're clawing our way up to modern ObjC slowly. > > Thank you for learning enough Cocoa to take this on! > > https://codereview.chromium.org/2472283002/diff/110001/content/common/font_li... > File content/common/font_list_mac.mm (right): > > https://codereview.chromium.org/2472283002/diff/110001/content/common/font_li... > content/common/font_list_mac.mm:20: [[[NSMutableDictionary alloc] init] > autorelease]; > Nit: you can use the convenience constructor: > > NSMutableDictionary* fonts_dict = [NSMutableDictionary dictionary]; @Avi Thank you for reviewing my code. I fixed it as your comment. :-)
The CQ bit was checked by corona10@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by corona10@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2472283002/#ps130001 (title: "nit fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== In chrome://settings/fonts, fonts are not alphabetically ordered. NSFontManager does not provide sorted font lists. So We should sort it manually. BUG=655594 ========== to ========== In chrome://settings/fonts, fonts are not alphabetically ordered. NSFontManager does not provide sorted font lists. So We should sort it manually. BUG=655594 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== In chrome://settings/fonts, fonts are not alphabetically ordered. NSFontManager does not provide sorted font lists. So We should sort it manually. BUG=655594 ========== to ========== In chrome://settings/fonts, fonts are not alphabetically ordered. NSFontManager does not provide sorted font lists. So We should sort it manually. BUG=655594 Committed: https://crrev.com/ac2a392c1984cb75dee0185b723a1f482aa7e77a Cr-Commit-Position: refs/heads/master@{#430048} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ac2a392c1984cb75dee0185b723a1f482aa7e77a Cr-Commit-Position: refs/heads/master@{#430048} |
