496385 - Number of bookmarks to remove should be in the middle ("Remove %d Bookmarks") inside the star panel

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Screenshot

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre ID:20090604031153

Right now we show the number of bookmarks which will be removed by pressing the remove bookmark button inside the star panel at the end of the button. For better readability we should move this in the middle of both words so it will look like: "Remove 2 Bookmarks".

Axel, you know if this would be possible for all the other locales?

Comment 1

[Axel Hecht [:Pike]]

This is WONTFIX, IIRC.

I think it was intentional to de-emphasize the number by putting it in braces, it's more of a "remove a bunch, and, yeah, a bunch would be 2 right now" than a "did you really intend to remove 2 and not 3?".

Don't have the original bug at my finger tips right now. Faarborg might remember.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> This is WONTFIX, IIRC.
> 
> I think it was intentional to de-emphasize the number by putting it in braces,
> it's more of a "remove a bunch, and, yeah, a bunch would be 2 right now" than a
> "did you really intend to remove 2 and not 3?".
> 
> Don't have the original bug at my finger tips right now. Faarborg might
> remember.

Alex, you can remember for such a decision and bug?

Comment 3

[Marco Bonardo [:mak]]

i remember the decision was exactly to use the current style Remove Bookmarks (n). that was probably bug 451586?

Comment 4

[Marco Bonardo [:mak]]

exactly
https://bugzilla.mozilla.org/show_bug.cgi?id=451586#c10

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mmh, Alex was not satisfied with the 'all' option and gave as a possible solution the current display. I feel that he should give a clear direction. If the way we have right now is still wanted we should close this bug as Wontfix.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

I think the only reason we didn't put it in the middle was for localization issues.  But if we want to do that for en-us, or other locals where it works, that is fine.

Comment 7

[Michael Kohler [:mkohler]]

Attachment: v1

(this patch is not tested since I don't succeed in having a bookmark counter on the star panel - it's always only "Remove Bookmark", I'm afraid)

Comment 8

[Marco Bonardo [:mak]]

you can get a counter in an easy way, take a bookmark and make 2, 3 copies of it. then open it, and click the star.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 381657 [details] [diff] [review]
v1

I don't see particular issues with localization here since we use plural forms. Just for sake of completeness and since there's no hurry to take this, i'm asking a preliminary l10n review.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 381657 [details] [diff] [review]
v1

The patch is technically OK, but I don't think it makes a lot of sense to push it for 3.5.

Also, an option with having the #1 at the end is a nice escape for locales with more plural forms.

Do you plan the patch for 3.5?

Comment 11

[Axel Hecht [:Pike]]

This is definitely not 3.5 material.

I wonder if we should investigate the more complete UI ideas from faaborg from bug 451586 comment 10 instead of patching the hack over the head.

Comment 12

[Marco Bonardo [:mak]]

nono, as i said there is no hurry here, this is not for 3.5 of course.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 381657 [details] [diff] [review]
v1

r+ for trunk, I'd add a note about option to place it at the end if placing it in the middle causes problems (due to multiple plural forms).

Comment 14

[Michael Kohler [:mkohler]]

Attachment: v1.1 (added a comment)

* Added a comment concerning the possible localization problems
* |editBookmark.removeBookmarks.label| in LOCALIZATION NOTE changed to |editBookmark.removeBookmarks2.label|

Comment 15

[Axel Hecht [:Pike]]

This is not reviewed enough to be checked in.

My personal take would be to not change the key names, but to update the localization note to offer both variants of writing the UI string.

Comment 11 hasn't been addressed yet, either.

Comment 16

[Michael Kohler [:mkohler]]

(In reply to comment #15)
> My personal take would be to not change the key names, but to update the
> localization note to offer both variants of writing the UI string.

And comment out the not used one?
 
> Comment 11 hasn't been addressed yet, either.

Do you mean the implementation of the whole more complex UI? Shouldn't we file a seperate bug for this and investigate if this is really wanted?

Comment 17

[Axel Hecht [:Pike]]

(In reply to comment #16)
> (In reply to comment #15)
> > My personal take would be to not change the key names, but to update the
> > localization note to offer both variants of writing the UI string.
> 
> And comment out the not used one?

Rereading the patch, the localization note in attachment 381779 [details] [diff] [review] is fine as is. Didn't dive into the specifics. I'd still leave the entity name, though.

> > Comment 11 hasn't been addressed yet, either.
> 
> Do you mean the implementation of the whole more complex UI? Shouldn't we file
> a seperate bug for this and investigate if this is really wanted?

If we can make this 0-cost (by not changing the entity name) for localizers, a separate bug would be fine.

That said, this patch still needs appropriate review to touch browser code, AFAICT. Some browser module peer would know which.

Comment 18

[Michael Kohler [:mkohler]]

Attachment: v2

Addressed comment 17.

Why is there no additional review field/dropdown?

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 390267 [details] [diff] [review]
v2

r+ for me from the l10n standpoint.

You probably need another review from browser/ui team.

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9dfacae57238

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090729 Minefield/3.6a1pre ID:20090729031626