Closed
Bug 954058
Opened 10 years ago
Closed 10 years ago
Move menu.xul.inc into an overlay
Categories
(Instantbird Graveyard :: Other, enhancement)
Instantbird Graveyard
Other
Tracking
(Not tracked)
RESOLVED
FIXED
0.3a1
People
(Reporter: bugzilla, Assigned: Mook)
Details
Attachments
(1 file, 3 obsolete files)
30.73 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 622 by Mook <mook.moz+bugs.instantbird AT gmail.com> at 2010-12-12 02:05:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Reporter | ||
Comment 1•10 years ago
|
||
*** Original post on bio 622 as attmnt 416 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 02:05:00 UTC *** Currently the various menus are in a preprocessed file included from multiple places (mostly on Mac since that requires each window to have a menu). This makes it hard for extensions to put up the default menu, since they don't have access to the build system. (Generally this means copying, which is pretty annoying and is likely to end up outdated.) It would be nice to be able to move all of that into an overlay xul instead so it's easier to import. Attached patch is a WIP; while it does the minimum it doesn't really sort out which places need what. It also still has a stub menu.xul.inc to import all the overlay targets (that should go away too).
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8352159 [details] [diff] [review] WIP *** Original change on bio 622 attmnt 416 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 02:06:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352159 -
Attachment is patch: true
Attachment #8352159 -
Attachment mime type: application/octet-stream → text/plain
Comment 3•10 years ago
|
||
*** Original post on bio 622 at 2010-12-12 02:23:55 UTC *** (In reply to comment #0) > Created an attachment (id=416) [details] > WIP > Attached patch is a WIP; while it does the minimum it doesn't really sort out > which places need what. I'm not sure of what you mean. On Mac currently the menus are identical on all Windows. > It also still has a stub menu.xul.inc to import all > the overlay targets (that should go away too). Do you see any better way to do it? It seems Firefox does it that way too with browserMountPoints.inc By the way, shouldn't the <?xml-stylesheet href="chrome://instantbird/skin/menus.css" type="text/css"?> lines be moved to menus.xul?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•10 years ago
|
||
*** Original post on bio 622 at 2010-12-12 11:00:35 UTC *** Ah, cool to see you cleaning up the menu problems. Maybe you can move the "Join chat" / "Account manager" shortcuts to a separate keyset that can be included on conversation windows as well (or whatever solution is suitable there) since we wanted these shortcuts to work from conversation as well. That's bug 953739 (bio 296) - Account manager shortcut bug 953859 (bio 418) - Keyboard shortcut to open "Join chat" dialog from conversation windows by the way. (There are other shortcut bugs for the conversation window but they are completely independent from this, so I think we can safely ignore them for now (namely bug 953882 (bio 441), bug 953933 (bio 496))).
Comment 5•10 years ago
|
||
*** Original post on bio 622 as attmnt 418 at 2010-12-12 15:36:00 UTC *** There are 2 benefits for the change you propose here: - let add-ons reuse the whole menu bar with a minimal amount of copy/paste. This is what you were interested in. - let overlays overlay the menus on all Windows at once on Mac. This will be great for add-ons, and offers a trivial way to fix the "Debug" menu of debug builds that currently works only in the buddy list. The additional change in the attached patch does this.
Reporter | ||
Comment 6•10 years ago
|
||
*** Original post on bio 622 as attmnt 419 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 19:10:00 UTC *** This has the last thing, plus splitting most of the platform-specific bits into separate overlays. With a little bit more change menus.xul wouldn't even need preprocessing... but the mac window menu bit is hard enough that I'd rather not think about it for now. Seems okay on Windows and Linux, but I don't have a Mac to test on. Are other platforms supported too that I should be aware of?
Attachment #8352162 -
Flags: review?(florian)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8352159 [details] [diff] [review] WIP *** Original change on bio 622 attmnt 416 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 19:10:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352159 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
*** Original post on bio 622 at 2010-12-12 19:17:54 UTC *** (In reply to comment #4) > Seems okay on Windows and Linux, but I don't have a Mac to test on. Are other > platforms supported too that I should be aware of? Currently, no. :-)
Comment 9•10 years ago
|
||
Comment on attachment 8352162 [details] [diff] [review] for review *** Original change on bio 622 attmnt 419 at 2010-12-12 22:43:58 UTC *** The patch I reviewed is attachment 8352162 [details] [diff] [review] (bio-attmnt 419) + additional changes discussed on IRC that are currently visible at http://sprunge.us/JiXU?diff. Overall, this is a great improvement. Some very small nits: - <!-- vim: set sw=2 --> lines hurt my eyes. Either remove them or move them before the license header (and keep an empty line before <!DOCTYPE). - In menus-mac.xul, add an empty line to separate the popupset block from the keyset block. - in menus.xul, <popupset id="mainpopupset"/> looks useless (it's already in menus.xul.inc). r=me with those nits fixed. Thanks for working on this!
Attachment #8352162 -
Flags: review?(florian) → review+
Reporter | ||
Comment 10•10 years ago
|
||
*** Original post on bio 622 as attmnt 421 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 22:52:00 UTC *** (I don't have editbugs, nor do I have commit privs, so you'll have to deal with it from here, I think)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8352162 [details] [diff] [review] for review *** Original change on bio 622 attmnt 419 by mook.moz+bugs.instantbird AT gmail.com at 2010-12-12 22:52:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352162 -
Attachment is obsolete: true
Comment 12•10 years ago
|
||
*** Original post on bio 622 at 2010-12-12 22:55:01 UTC *** (In reply to comment #7) > (I don't have editbugs No longer true ;).
Comment 13•10 years ago
|
||
*** Original post on bio 622 at 2010-12-12 23:35:41 UTC *** Fixed, https://hg.instantbird.org/instantbird/rev/45702da127b7 Thanks!
Assignee: nobody → bugzilla
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a1
Comment 14•10 years ago
|
||
Comment on attachment 8352164 [details] [diff] [review] address review comments *** Original change on bio 622 attmnt 421 at 2010-12-13 14:10:50 UTC *** Fixing the review flag to please Quentin...
Attachment #8352164 -
Flags: review+
Comment 15•10 years ago
|
||
Comment on attachment 8352161 [details] [diff] [review] additional change *** Original change on bio 622 attmnt 418 at 2010-12-13 14:11:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352161 -
Attachment is obsolete: true
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8352161 [details] [diff] [review] additional change *** Original change on bio 622 attmnt 418 by tymerkaev AT gmail.com at 2010-12-13 15:02:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352161 -
Attachment description: additionnal change → additional change
Comment 17•10 years ago
|
||
*** Original post on bio 622 at 2010-12-16 07:01:09 UTC *** Just pushed a regression fix: https://hg.instantbird.org/instantbird/rev/783fc0cf669c (the 'Check for Updates...' menu item was broken on Mac). I don't know how I missed that during the review/testing...
Reporter | ||
Updated•10 years ago
|
Assignee: bugzilla → mook.moz+mozbz
You need to log in
before you can comment on or make changes to this bug.
Description
•