Open Bug 661055 Opened 13 years ago Updated 1 year ago

Show number of windows/tabs for session restore

Categories

(Firefox :: Session Restore, enhancement)

Product:
Firefox
Component:
Session Restore
Platform:
x86
macOS
Type:
enhancement

Tracking

()

People

(Reporter: limi, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug])

Attachments

(3 files)

Bug 661055 - Add restorableObjectsCount getter to SessionStore.jsm
59 bytes, text/x-review-board-request
Details
Bug 661055 - Display number of windows and tabs to be restored from previous session.
59 bytes, text/x-review-board-request
Details
test WIP
59 bytes, text/x-review-board-request
Details 
The menu entry and the session restore button on about:home should indicate how many windows and tabs you end up with if you click them.

Something like "Restore previous N tabs & M windows" would do the trick.
Assignee: paul → nobody
Severity: normal → enhancement
Mentor: mdeboer
Whiteboard: [good next bug]
So I've got an initial implementation that displays this on the menu items in the History menu and the appMenu (hamburger).  Haven't done about:home yet since the new about:home doesn't have a session restore button, and also haven't done the new hidden restore button in the tab bar that one activates by setting browser.tabs.restorebutton to 1, though at least the latter would be very easy to add (right next to where I've been modifying browser.js).
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Attachment #8914121 - Flags: feedback?(mdeboer)
Attachment #8914122 - Flags: feedback?(mdeboer)
Comment on attachment 8914121 [details]
Bug 661055 - Add restorableObjectsCount getter to SessionStore.jsm

https://reviewboard.mozilla.org/r/185442/#review191436

f+! This looks like a very reasonable addition to me! It's only missing some documentation in docblock/ JSDoc comments and a - should be simple - unit test.

::: browser/components/sessionstore/SessionStore.jsm:212
(Diff revision 1)
>   * This value is controlled by preference privacy.resistFingerprinting.
>   */
>  var gResistFingerprintingEnabled = false;
>  
>  this.SessionStore = {
> +  get restorableCounts() {

nit: I'd rename this to 'restorableObjectsCount'. We use the generic, abstract name 'objects' to refer to tab & window objects together elsewhere in this code.
Attachment #8914121 - Flags: feedback?(mdeboer) → feedback+
Comment on attachment 8914122 [details]
Bug 661055 - Display number of windows and tabs to be restored from previous session.

https://reviewboard.mozilla.org/r/185444/#review191454

::: browser/locales/en-US/chrome/browser/browser.properties:403
(Diff revision 1)
>  menuUndoCloseWindowSingleTabLabel=#1
> +# LOCALIZATION NOTE (menuRestoreLastSession): Replaces historyRestoreLastSession.label,
> +# and appMenuHistory.restoreSession.label from browser.dtd with counts
> +# %1$S is the no. of windows %2$S is the windows plural form, %3$S is the number
> +# of tabs %4$S is the tabs plural form (e.g. Restore previous 2 windows and 7 tabs)
> +menuRestoreLastSession=Restore previous %1$S %2$S and %3$S %4$S

Unfortunately, this structure may work with English and a handful of other languages, but not for all. The only way to make this work properly is not Limi's suggestion as described in comment 0, but more something like:

1. Keep the 'historyRestoreLastSession.label', but perhaps move it to browser.properties and call it 'menuRestoreLastSessionLabel',
2. Change the label to 'Restore Previous Session #1'
3. #1 will be replaced by 'menuRestoreLastSessionCount=#1/ #2'
4. In which #1 will be replaced by 'menuRestoreLastSessionWindowCount=#1 window;#1 windows',
5. And #2 will be replaced by 'menuRestoreLastSessionTabCount=#1 tab;#1 tabs'.
6. Remove the labels from the menuitem and toolbarbutton, since they're going to be overridden by the one on the command element, and the unused string(s) from browser.dtd.

Does this make sense to you? Please let me know if you have any questions!
Attachment #8914122 - Flags: feedback?(mdeboer)
Comment on attachment 8914122 [details]
Bug 661055 - Display number of windows and tabs to be restored from previous session.

https://reviewboard.mozilla.org/r/185444/#review191454

> Unfortunately, this structure may work with English and a handful of other languages, but not for all. The only way to make this work properly is not Limi's suggestion as described in comment 0, but more something like:
> 
> 1. Keep the 'historyRestoreLastSession.label', but perhaps move it to browser.properties and call it 'menuRestoreLastSessionLabel',
> 2. Change the label to 'Restore Previous Session #1'
> 3. #1 will be replaced by 'menuRestoreLastSessionCount=#1/ #2'
> 4. In which #1 will be replaced by 'menuRestoreLastSessionWindowCount=#1 window;#1 windows',
> 5. And #2 will be replaced by 'menuRestoreLastSessionTabCount=#1 tab;#1 tabs'.
> 6. Remove the labels from the menuitem and toolbarbutton, since they're going to be overridden by the one on the command element, and the unused string(s) from browser.dtd.
> 
> Does this make sense to you? Please let me know if you have any questions!

My first question is which scenarios would my current approach fail?  Are there languages that vary the order of the count and word depending on the number?  ie. could you have 'menuRestoreLastSessionTabCount=tab \#1;\#2 tabs' Otherwise I'd have though the ability to independently order all four of (tab|window) (word|count) would be enough.

Is it not worth keeping around a default label, in case this code fails somehow and doesn't set the label?

Also when there is only one window to restore is it worth dropping the mention of window and only presenting tab count, especially as it gets merged with the existing window?
It's not just order, it's stuff such as case inflections etc.  There's an enlightening article about the various use cases in various languages in this article (which describes a Perl solution, but the linguistic points are still striking and pertinent here): http://search.cpan.org/dist/Locale-Maketext/lib/Locale/Maketext/TPJ13.pod
(In reply to Ian Moody [:Kwan] from comment #6)
> My first question is which scenarios would my current approach fail?  Are
> there languages that vary the order of the count and word depending on the
> number?  ie. could you have 'menuRestoreLastSessionTabCount=tab \#1;\#2
> tabs' Otherwise I'd have though the ability to independently order all four
> of (tab|window) (word|count) would be enough.

I think the link that era provides is an excellent resource. We have to think of so many variations and my suggestion would cause the least amount of trouble of our l10n friends, I think.

> Is it not worth keeping around a default label, in case this code fails
> somehow and doesn't set the label?

Good point, although I think that we're in a different category of trouble when this code fails, but ok. :-P
Can you perhaps at least eliminate one of the two?

> Also when there is only one window to restore is it worth dropping the
> mention of window and only presenting tab count, especially as it gets
> merged with the existing window?

Yes, I think that's a good idea.
(In reply to era+mozilla from comment #7)
> It's not just order, it's stuff such as case inflections etc.  There's an
> enlightening article about the various use cases in various languages in
> this article (which describes a Perl solution, but the linguistic points are
> still striking and pertinent here):
> http://search.cpan.org/dist/Locale-Maketext/lib/Locale/Maketext/TPJ13.pod

Oh, I thought I'd handled such things by using PluralForm for "window(s)" and "tab(s)", but I guess not.  Still more to learn! Languages are hard :S
Thanks for the article!  Quite interesting, even if I don't understand half the technical terms.

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to Ian Moody [:Kwan] from comment #6)
> > My first question is which scenarios would my current approach fail?  Are
> > there languages that vary the order of the count and word depending on the
> > number?  ie. could you have 'menuRestoreLastSessionTabCount=tab \#1;\#2
> > tabs' Otherwise I'd have though the ability to independently order all four
> > of (tab|window) (word|count) would be enough.
> 
> I think the link that era provides is an excellent resource. We have to
> think of so many variations and my suggestion would cause the least amount
> of trouble of our l10n friends, I think.

No problem, I'll switch your suggestion.

> > Is it not worth keeping around a default label, in case this code fails
> > somehow and doesn't set the label?
> 
> Good point, although I think that we're in a different category of trouble
> when this code fails, but ok. :-P
> Can you perhaps at least eliminate one of the two?

Sure.  I did wonder why it was duplicated in the first place.  I'll see if the original bug mentions.
(In reply to Ian Moody [:Kwan] from comment #9)
> Sure.  I did wonder why it was duplicated in the first place.  I'll see if
> the original bug mentions.

I think I was the one who introduced that duplicate string ;-) Feel free to remove it.
Sorry for the lag, I got a bit stuck on the test, and then distracted by other things.

I'm afraid I can't figure out how to get SessionStore initialised so that the function works, so I'm looking for some pointers.

I see that you're on PTO at the moment though so I'll wait to needinfo you until after it's over.
Flags: needinfo?(mdeboer)
Hi Ian! I can see why you can't get the test to work: SessionStore.jsm initialization relies on various browser-specific mechanics to be present and running, which means that you have to write a browser-chrome test instead of an xpcshell test.

You can find many examples of this type of test in the 'browser/components/sessionstore/test/' folder - they're all prefixed with 'browser_', like 'browser_capabilities.js'.
I recommend checking a couple of these test (the most recently added ones, for example) and use them as a starting point for your own unit test.
Flags: needinfo?(mdeboer)
Mentor: mdeboer
Severity: normal → S3

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: moz-ian → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: