Closed Bug 581661 Opened 10 years ago Closed 8 years ago

On Windows, users should be able to hide the Thunderbird Menu Bar and show it with the alt key

Categories

(Thunderbird :: Mail Window Front End, enhancement)

x86
Windows 7
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 9.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3pre) Gecko/20100723 Minefield/4.0b3pre
Build Identifier: 

See also the Firefox Bug 456535. This would also help Bug 569400 (Aero Glass on Thunderbird)

Reproducible: Always
This patch is a copy paste from the Firefox patch. It only works in Main window. In Addressbook and Composer the menu entries appears but the menu does not hide.

I'm not a coder, so if someone with experience can take this.
I tried the patch again and deleted the localstore.rdf. Now it works also in Addressbook and Composer.

But if you need tests, then a developer should take this Bug to create the needed tests.
Attachment #460029 - Attachment is obsolete: true
awesome idea
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
If you want your patch to get thru you'll need to ask for reviews see https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements
Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

I hope I took the right one.

If this patch need automated tests, I hope someone takes this Bug and writes them. I'm not a programmer and I don't know how to write them.

I tried to assign this Bug to me. But I have no rights to do this or I don't know how to do.
Attachment #460121 - Flags: review?(dmose)
Assignee: nobody → richard.marti
I assigned it to you.
Richard, thanks so much for the patch!

I've adjusted your Bugzilla privs so that you will be able to edit bugs and assign them to yourself in the future.

This patch will need tests before it can land.  Jim, I don't suppose you'd have any interest in putting together an automated test for this bug?
Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

Removing the review request until we have a patch with ui-review and tests.  Once we do get to that patch, I suspect bwinton is probably the most suitable reviewer here...
Attachment #460121 - Flags: review?(dmose) → ui-review?(clarkbw)
(In reply to comment #8)
> This patch will need tests before it can land.  Jim, I don't suppose you'd have
> any interest in putting together an automated test for this bug?

I haven't really looked at the patch. If it's platform-independent, I can probably do it, but I don't have access to a Windows Vista/7 machine (and my XP machine isn't set up to compile Mozilla stuff).
Jim, maybe if you remove the #ifdef XP_WIN in the .xul files it should also work on other platforms.
Once this gets ui-review, please remind me and I'll try and find someone to help out with the unit test.
Keywords: helpwanted
Whiteboard: [needs unit test]
Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

"Stealing" Bryan's UI-review (after asking him on IRC)
Attachment #460121 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Blocks: tb-netbooks
For some odd reason I got the extension to work, but not the patch. Investigating what could be going on.
Did you test it with AeroGlass theme? Then remove the -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");

With this in the theme the autohide does not work. If you don't use this theme, then try it with deleting or renaming of the localstore.rdf.
Comment on attachment 460121 [details] [diff] [review]
Patch with corrected line endings CR LF -> LF

That did the trick!
It's good to see that it's needs an initial trigger from a right click menu in order to work, meaning that it won't be triggered by people who don't know of the function.
ui-r+!
Attachment #460121 - Flags: ui-review?(nisses.mail) → ui-review+
Flags: in-testsuite?
Why was review request not requested ?
(In reply to comment #19)
> Why was review request not requested ?

Because I don't know how to write the required automated test and nobody offered to do it.
Richard,

I could probably help you write the tests.  Can you ping me (bwinton) on IRC sometime today?

(Also, I'm assigning it to you, cause it looks like you've done most of the work on it already.  ;)

Later,
Blake.
Status: NEW → ASSIGNED
Blocks: 645294
Blocks: 667241
Attached patch WIP patch with tests (obsolete) — Splinter Review
Just for tracking purposes, here are some tests I wrote for this bug. However, not having a Windows dev environment, I can't actually test the tests, so hopefully someone can step in and polish this off. :)
Why do you limit that to Windows? Firefox allows to hide the menubar on Linux as well (and displays the Firefox button).
(In reply to comment #23)
> Why do you limit that to Windows? Firefox allows to hide the menubar on
> Linux as well (and displays the Firefox button).

On Vista/Win7 it is usual habit to hide the menubar, on Linux not. But FX has the AppMenu button which is showing when the menubar is hidden. TB don't have this ATM and this would it make hard to discover the new behaviour on Linux.
Comment on attachment 543870 [details] [diff] [review]
WIP patch with tests

Blake can you run the tests and check if they are working?
If they are working, this is also a r? ;)
Attachment #543870 - Flags: ui-review+
Attachment #543870 - Flags: review?(bwinton)
Attachment #460121 - Attachment is obsolete: true
(In reply to comment #23)
> Why do you limit that to Windows? Firefox allows to hide the menubar on
> Linux as well (and displays the Firefox button).

That's dependent on having a Thunderbird button, which is beyond the scope of this bug.
(In reply to comment #26)

> That's dependent on having a Thunderbird button, which is beyond the scope
> of this bug.

Do we have a bug for that?
I figured out why the tests aren't working: to summarize, the click calls don't flip the checkmark, so our code gets confused. See bug 670829 and bug 670830.
Depends on: 670830
Wow, we still have lots of issues with our tests :/

I've made this depend on MozMill 1.5 because it seemed to be the most convenient thing to do.
Attachment #543870 - Attachment is obsolete: true
Attachment #545613 - Flags: review?(bwinton)
Attachment #543870 - Flags: review?(bwinton)
Depends on: 656736
Comment on attachment 545613 [details] [diff] [review]
patch with working tests

Review of attachment 545613 [details] [diff] [review]:
-----------------------------------------------------------------

Other than those small nits, and the fact that I can't run the tests because we're in the middle of upgrading MozMill, I like it!  ;)

So, r=me with those fixed, and the test infrastructure upgrade finished.

Thanks,
Blake.

::: mail/base/content/mailCore.js
@@ -269,2 +269,4 @@
> > -  toolbar.collapsed = aEvent.originalTarget.getAttribute("checked") != "true";
> > +  var hidingAttribute = toolbar.getAttribute("type") == "menubar" ?
> > -  document.persist(toolbar.id, "collapsed");
> > +                        "autohide" : "collapsed";
> > +
> > +  dump("final checked = " + aEvent.originalTarget.getAttribute("checked"));

We should probably remove the dump statement here.

::: mail/test/mozmill/message-window/test-autohide-menubar.js
@@ +18,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2011
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Jim Porter <squibblyflabbetydoo@gmail.com>

Should this be you or Squib?

@@ +67,5 @@
> + *
> + * @param controller the mozmill controller for the window
> + * @param elem the element to click on (usually the menubar)
> + * @param hide true to hide, false otherwise
> + * @return true if the menubar's autohide attribute could be set

This function doesn't seem to ever return true!

@@ +90,5 @@
> +function help_test_autohide(controller, menubar) {
> +  function hiddenChecker(aHidden) {
> +    // The XUL hidden attribute isn't what is set, so it's useless here -- use
> +    // information from the box model instead.
> +    return function () ((menubar.getBoundingClientRect().height != 0) ^ aHidden);

I think I'ld prefer "==" to "^".  (Or is that "!="?)
Attachment #545613 - Flags: review?(bwinton) → review+
https://hg.mozilla.org/comm-central/rev/8bbd7b60e5a8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [needs unit test]
Target Milestone: --- → Thunderbird 9.0
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
No idea what happened...
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Pushed a followup adding the tests I forgot to add in the previous push: https://hg.mozilla.org/comm-central/rev/e5a042f2e020

No idea how I missed them though :(
(In reply to David :Bienvenu from comment #27)
> (In reply to comment #26)
> 
> > That's dependent on having a Thunderbird button, which is beyond the scope
> > of this bug.
> 
> Do we have a bug for that?

https://bugzilla.mozilla.org/show_bug.cgi?id=650170
Depends on: 716481
Depends on: 725554
No longer depends on: 716481
You need to log in before you can comment on or make changes to this bug.