Closed
Bug 581661
Opened 14 years ago
Closed 13 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)
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)
16.58 KB,
patch
|
bwinton
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Comment 4•14 years ago
|
||
awesome idea
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•14 years ago
|
||
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
Assignee | ||
Comment 6•14 years ago
|
||
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)
Updated•14 years ago
|
Assignee: nobody → richard.marti
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
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)
Comment 10•14 years ago
|
||
(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).
Assignee | ||
Comment 11•14 years ago
|
||
Jim, maybe if you remove the #ifdef XP_WIN in the .xul files it should also work on other platforms.
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
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)
Comment 14•14 years ago
|
||
also already available via https://addons.mozilla.org/en-US/thunderbird/addon/4762/ if the write up is correct
Updated•14 years ago
|
Blocks: tb-netbooks
Comment 15•14 years ago
|
||
For some odd reason I got the extension to work, but not the patch. Investigating what could be going on.
Assignee | ||
Comment 16•14 years ago
|
||
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 17•14 years ago
|
||
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+
Updated•14 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 20•14 years ago
|
||
(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.
Comment 21•14 years ago
|
||
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
Comment 22•13 years ago
|
||
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. :)
Comment 23•13 years ago
|
||
Why do you limit that to Windows? Firefox allows to hide the menubar on Linux as well (and displays the Firefox button).
Assignee | ||
Comment 24•13 years ago
|
||
(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.
Assignee | ||
Comment 25•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #460121 -
Attachment is obsolete: true
Comment 26•13 years ago
|
||
(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.
Comment 27•13 years ago
|
||
(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?
Comment 28•13 years ago
|
||
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.
Comment 29•13 years ago
|
||
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)
Comment 30•13 years ago
|
||
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+
Comment 31•13 years ago
|
||
https://hg.mozilla.org/comm-central/rev/8bbd7b60e5a8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [needs unit test]
Target Milestone: --- → Thunderbird 9.0
Updated•13 years ago
|
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Comment 32•13 years ago
|
||
No idea what happened...
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 33•13 years ago
|
||
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 :(
Comment 34•13 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•