Closed Bug 954058 Opened 10 years ago Closed 10 years ago

Move menu.xul.inc into an overlay

Categories

(Instantbird Graveyard :: Other, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: Mook)

Details

Attachments

(1 file, 3 obsolete files)

*** 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 ***
Attached patch WIP (obsolete) — Splinter Review
*** 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).
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
*** 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
*** 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))).
Attached patch additional change (obsolete) — Splinter Review
*** 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.
Attached patch for review (obsolete) — Splinter Review
*** 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)
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
*** 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 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+
*** 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)
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
*** 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 ;).
*** 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 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 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
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
*** 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...
Assignee: bugzilla → mook.moz+mozbz
You need to log in before you can comment on or make changes to this bug.