306018 - [ui polish] Move "Check for Updates" into Application menu on OSX

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

While bug 288322 is trying to make it easier in general to add items to the OS X
application menu, it's fairly important for us to get the "Check for Updates..."
menuitem moved into there for FFx 1.5 since that's where *every* other OS X
application puts this function.

Comment 1

[Asaf Romano (gone)]

We can do a band-aid here (and for "Clear Private Data" too). However, I'm a bit
questioning the value of it.

Comment 2

[Asaf Romano (gone)]

taking per IRC.

Comment 3

[jhp (no longer active)]

Attachment: patch

Moves the update menu item from the 'Help' menu to the app menu.

Comment 4

[jhp (no longer active)]

Are you sure about moving "Clear Private Data" also?  Can't it just stay in 'Tools'?

Comment 5

[Asaf Romano (gone)]

For 1.5, "Tools" will do.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 194252 [details] [diff] [review]
patch

Please update tbird's help menu as well; otherwise, the menu item will be
exposed in both menus

>+    // This code reads the "label" attribute from the <menuitem/> with
>+    // id="aboutName" and id="checkForUpdates", and puts their labels in
>+    // the Apple Menu

s/Apple/Application (yeah, i know the rest of this file is very outdated, not
just label-wise). Also, please add a hack note with a reference to this bug.

Since I'm going to rework this on the trunk, r=me for 1.8 branch only with
those addressed.

Comment 7

[Jo Hermans]

(In reply to comment #4)
> Are you sure about moving "Clear Private Data" also?  Can't it just stay in
'Tools'?

That would be bug 283791 if it was fixed also.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 194252 [details] [diff] [review]
patch

Actually, this doesn't play well with the live menu label ("Downloading",
"Installing", etc.)

Minusing, and recommending to push this to 2.0.

Comment 9

[Marcia Knous [:marcia]]

mano: cbeard was asking more about why this would be difficult to implement for
1.5.  Can you a more in depth explanation of what would be involved to do this,
and what risks we would run?  thanks.

Comment 10

[jhp (no longer active)]

(In reply to comment #8)
> Actually, this doesn't play well with the live menu label ("Downloading",
> "Installing", etc.)

Can you explain this more?  What is the problem?

Comment 11

[Asaf Romano (gone)]

The menuitem label is being changed during runtime (for example,to
"Downloading..." while an update is being downloaded); however, the application
menu is only built once, so the menuitem label is always "Check For Updates".

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Of the two, I my preference would be for it to be in the application menu with a
static label reading "Check for Updates...". IMO, it's better to have the
function in a place where people are used to finding it than to have it in an
unusual place. A static label isn't the greatest, but I don't think it's
seriously confusing for users who've hidden the "Check for Updates..." window to
hit that same menu item again to get it back, and hopefully we can fix it to be
dynamic in the future. The dynamic label is also only useful in secondary use-cases:

Primary case: 
 - user selects "Check for Updates..." and follows wizard-like UI to download
update and apply

Secondary cases:
 - user dismisses Check for Updates (non-modal) dialog using "Hide" button
 - after dismissing the immediate update using the "Later" button, user wants to
apply updates using the dialog instead of just restarting

Comment 13

[Jan Moesen]

Of the applications I have running now, the three I checked (iPhoto, iTunes, VLC) have their "Check for updates" item in the application menu. Their might be an Apple HIG about it, but I do not know off-hand. It seems to be the most common place for it to be, though.

Arguably also the best: I just went to my Thunderbird's "Help" menu and it had an item that said "Downloading Thunderbird $build_id". That seems very odd wording for a help menu item, and rather out of place. I prefer having a static menu title in the application menu to a dynamic one in the wrong menu.

If you want, I can check if this patch still applies and update it if necessary, next week.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #11)
> The menuitem label is being changed during runtime (for example,to
> "Downloading..." while an update is being downloaded); however, the application
> menu is only built once, so the menuitem label is always "Check For Updates".

After thinking about this more (like, for the past year and a bit) I think I'm actually OK with losing that bit of dynamic state on OSX. The trade off in terms of putting the function where users expect seems like a clear win to me.

Comment 15

[Ed Lee :Mardak]

(In reply to comment #6)
> (From update of attachment 194252 [details] [diff] [review])
> Please update tbird's help menu as well; otherwise, the menu item will be
> exposed in both menus
Would it be bad to have it in both menus? Helps users that expect it to be under the app menu and keeps it consistent with what existing users expect with the updating text.

Comment 19

[Robert Strong (they/them - no direct email)]

Moving over to Firefox -> Menus since that is what needs to be modified.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Joe said he was interested in taking this ...

Comment 22

[Marc E.]

It is been 4 years?

Firefox 3.5 is out.

What a joke.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mano, you wanna still work on this bug? Otherwise we should leave it for another person.

Comment 24

[Johnathan Nightingale [:johnath]]

Much as I would personally save dozens of clicks per year with this change, we would not hold the 3.6 release for it. blocking-

Re-assigning the owner too, so that others feel free to pick it up - I would like to see it fixed.

Comment 25

[d]

Firefox 3.7 will include major UI changes. If you think compatibility with other applications on the OS important, make sure to get it in for 3.7. Personally it doesn't matter to me, it will be a simple matter of getting used to the new location of the updater menu.

Comment 27

[Timo]

I think that in OSX this is just one of the things that should be changed in a following release.

There has put a lot of thought in the UI structure of OSX, so why not use it and make Firefox as native as possible in every way. There are lots of guidelines on how to design your interfaces.

Another thing that I found kind of awkward was the fact that Firefox has a completely different GUI (or default theme) than native Mac apps.
I am using the theme Grapple now, which looks fine, but wouldn't it be a smart move to make the default theme in Mac OSX more native than the current Silverlight?

Comment 28

[Markus Amalthea Magnuson]

Attachment: Move the software update item from Help menu to application menu on Mac

Here's a new patch to move the software update item to the application menu on Mac. The original item is hidden but it's node is preserved and then used for constructing a new item below the "About Firefox" item in the app menu.

Things seem to work out, however, a new browser window is created when selecting that new menu item. This happens each consecutive time, and I am not really sure why.

Hoping someone with a bit more knowledge of the mozilla code could give me some hints on this one.

Comment 29

[Gordon Mei]

The current stable build of Firefox (3.5.7) and the release candidate for 3.6 still have "Check for updates..." under the Help menu, where it doesn't belong.

"The application menu contains items that apply to the application as a whole rather than to a specific document or other window." -Apple HIG

I'd say Firefox updates apply to the entire application, thus are not document-specific.

The Application Menu:
http://developer.apple.com/mac/library/DOCUMENTATION/UserExperience/Conceptual/AppleHIGuidelines/XHIGMenus/XHIGMenus.html#//apple_ref/doc/uid/TP30000356-CIHDGFJB

The Help Menu:
http://developer.apple.com/mac/library/DOCUMENTATION/UserExperience/Conceptual/AppleHIGuidelines/XHIGMenus/XHIGMenus.html#//apple_ref/doc/uid/TP30000356-TPXREF108

Comment 30

[Josh Aas]

I want to review this patch before it goes in, request when you're ready.

Comment 31

[d]

(In reply to comment #29)
> The current stable build of Firefox (3.5.7) and the release candidate for 3.6
> still have "Check for updates..." under the Help menu, where it doesn't belong.

If you look at the post above yours, you'll see that a patch was just posted a few hours earlier than your post. If it gets checked in soon, it will be in Firefox 3.7.

Comment 32

[Asaf Romano (gone)]

Comment on attachment 421625 [details] [diff] [review]
Move the software update item from Help menu to application menu on Mac

As I said a while ago, this hardcoded list should go away (by the way, thunderbird wants "Account Settings" moved in there too).  We should find out some other solution.  A <menu> element in the hidden window might work.

Comment 33

[Josh Aas]

I agree with Mano in the long run but I don't think it needs to hold this up.

Comment 34

[Markus Amalthea Magnuson]

(In reply to comment #32)
> (From update of attachment 421625 [details] [diff] [review])
> As I said a while ago, this hardcoded list should go away (by the way,
> thunderbird wants "Account Settings" moved in there too).  We should find out
> some other solution.  A <menu> element in the hidden window might work.

Does it mean I should make the changes in the actual .xul files (using #ifdefs) containing the menu elements instead?

Comment 35

[Josh Aas]

You'd have to rewrite our system for populating the application menu in our Mac OS X menu code. Not worth doing right now when you have this patch ready to go.

Comment 36

[Josh Aas]

Comment on attachment 421625 [details] [diff] [review]
Move the software update item from Help menu to application menu on Mac

>-      // Add separator after About menu
>-      [sApplicationMenu addItem:[NSMenuItem separatorItem]];      
>     }
>-    
>+
>+    // Add the software update menu item
>+    itemBeingAdded = CreateNativeAppMenuItem(inMenu, NS_LITERAL_STRING("checkForUpdates"), @selector(menuItemHit:),
>+                                             eCommand_ID_Update, nsMenuBarX::sNativeEventTarget);
>+    if (itemBeingAdded) {
>+      [sApplicationMenu addItem:itemBeingAdded];
>+      [itemBeingAdded release];
>+      itemBeingAdded = nil;
>+
>+      // Add separator after software update menu item
>+      [sApplicationMenu addItem:[NSMenuItem separatorItem]];

The only code problem I see with your patch is that if an application has an about menu but no update menu then we'll be missing a separator. Handle this like the hide/show items below it, keeping track of whether or not you need a separator.

Comment 37

[Markus Amalthea Magnuson]

Attachment: Move the software update item from Help menu to application menu on Mac.

Added check for separator, as mentioned in comment 26. Why do we use TRUE/FALSE for the BOOLs by the way? Shouldn't it be YES/NO in Obj-C, or are things different in Obj-C++ and/or our codebase?

Josh, the problem still persists where a new browser window is opened each time you use the "Check for Updated…" menu item. Do you have any idea why this happens, when it doesn't for e.g. the About item?

Comment 38

[Markus Amalthea Magnuson]

Uhm, as mentioned in comment 36, that is.

Comment 39

[Asaf Romano (gone)]

Yeah, let's Just

Comment 40

[Asaf Romano (gone)]

Yeah, let's Just Say That every time we need yet another item in the application menu  (or even when we rewrite the entire mac widget code!) and never fix the bug.

I also don't understand at all why would it require a rewrite.

Comment 41

[Markus Amalthea Magnuson]

(In reply to comment #40)
> Yeah, let's Just Say That every time we need yet another item in the
> application menu  (or even when we rewrite the entire mac widget code!) and
> never fix the bug.
> 
> I also don't understand at all why would it require a rewrite.

Would a better solution be something like I described in comment 34?

Comment 42

[Asaf Romano (gone)]

That's pretty much what I meant (in addition to moving the items to hiddenWindow.xul, which is the first xul-window that's loaded in mac-gecko).

Comment 43

[Markus Amalthea Magnuson]

(In reply to comment #42)
> That's pretty much what I meant (in addition to moving the items to
> hiddenWindow.xul, which is the first xul-window that's loaded in mac-gecko).

Will make a new patch using this method instead.

Comment 44

[Asaf Romano (gone)]

I prefer this syntax, btw.
<menu appmenu="true">
  <menutiem ...>
 <...>
</menu>

It won't hurt other platforms, because hiddenWindow.xul is always hidden.  Also, I don't mind if you limit the number of items (if necessary) or if you do add some other magic (say, special position for the first two items or having always having a separator between them etc). However, please do not: 1. Use special IDs 2. Use <menu appmenu="true"> from anything but the hiddenWindow.

Thanks!

Comment 45

[Josh Aas]

(In reply to comment #40)
> Yeah, let's Just Say That every time we need yet another item in the
> application menu  (or even when we rewrite the entire mac widget code!) and
> never fix the bug.

I'm sure you understand that we don't always have time to do everything we'd like to do. I want what's best for this code, please don't imply that I don't because you're frustrated about a particular (admittedly low-priority) feature.

> I also don't understand at all why would it require a rewrite.

You'll have to rewrite the special item ID system, which will require significantly more work than the existing patch required. If someone has time to do this work I'd certainly appreciate it but as I said before, I don't think it needs to be a blocker for this feature.

Comment 46

[Markus Amalthea Magnuson]

OK, seriously. You need to decide on some strategy here before I write any more code. I am not going to put time into patches that get stuck because of project philosophy discussions.

Comment 47

[Josh Aas]

Markus - I will accept either patch/approach. If you're willing to write the more difficult one, which would allow for arbitrary Application menu items, it would be appreciated. It's your call since it is your time and effort. Sorry if that was unclear and thanks for your help either way.

Comment 48

[Markus Amalthea Magnuson]

(In reply to comment #47)
> Markus - I will accept either patch/approach. If you're willing to write the
> more difficult one, which would allow for arbitrary Application menu items, it
> would be appreciated. It's your call since it is your time and effort. Sorry if
> that was unclear and thanks for your help either way.

I will have another look at the approach using hiddenWindow.xul etcetera, I'll let you know when I have something.

Comment 49

[Markus Amalthea Magnuson]

Attachment: Move the software update item from Help menu to application menu on Mac.

I have considered various solutions, but conclusively I do not have enough in-depth knowledge of xul and mozilla internals in general, so I will pursue my original approach.

I have updated the previous patch to once again apply without errors to the current tree. However, the bug still persists: an empty browser window opens when I choose "Check for Updates..." However, new additional windows no longer open every time, but the previously opened empty window is reused. The bug is thus only visible if no windows are open when triggering the menu item. Other changes in the tree must have caused this change of behavior.

Removing the javascript target method (i.e. oncommand="checkForUpdates();") still makes the empty window disappear, so I figure something strange is going on either at xul level, but more probably in the obj-c++ code.

Comment 50

[Josh Aas]

Comment on attachment 429528 [details] [diff] [review]
Move the software update item from Help menu to application menu on Mac.

Lets have Mano review first - he's more likely to be helpful in understanding why checkForUpdates opens new windows in certain cases.

Mano - I know you don't like this patch but unless someone has time to rewrite the system I think we should do this. I might be able to do that in a bit but I can't prioritize it right now.

Comment 51

[Markus Amalthea Magnuson]

So, nothing's gonna happen with this patch? Should I spend any more energy on it?

Comment 52

[Asaf Romano (gone)]

I'll look into this next week.  I cannot promise that I know checkForUpdates better than josh though.

Comment 53

[José Jeria]

Will this patch make it into Firefox 4? The review has been pending for 4 months now.

Comment 54

[[not reading bugmail]]

Well, depends if the design direction is now moving it to about:Dialog

Comment 55

[Markus Amalthea Magnuson]

(In reply to comment #54)
> Well, depends if the design direction is now moving it to about:Dialog
No, why would it be moved there? The whole point of this change is to move the software update menu item to where it is supposed to be according to Mac standards.

I'll have a look at the patch to see that it still applies cleanly, but I would really like it reviewed properly and/or pushed sooner or later. It is a small, quick fix for a completely obvious UI change.

Comment 56

[[not reading bugmail]]

(In reply to comment #55)
> (In reply to comment #54)
> > Well, depends if the design direction is now moving it to about:Dialog
> No, why would it be moved there? The whole point of this change is to move the
> software update menu item to where it is supposed to be according to Mac
> standards.

Ok, thanks for clarification.  Windows doesn't seem to have its own standard but Faaborg recently mentioned it for the Firefox Menu so didn't know if Mac would follow suit or not even since he has give ui r+ a while back.

Comment 57

[Markus Amalthea Magnuson]

Patch still applies, strange window behavior still present.

Comment 59

[Rimas Kudelis [use needinfo to ping]]

Markus: since Mano isn't answering, perhaps you should ask for Josh's review instead?

Comment 61

[Markus Amalthea Magnuson]

A small note: I discussed this bug with Josh recently, I will have to produce a bug free patch before he or anyone else can review it properly. Any help is welcome, otherwise I will try to delve deeper into the source to find out what is causing the extra window behavior.

Comment 62

[Asaf Romano (gone)]

An updated version of your patch is on its way.

The problem was here:
 enum {
-  eCommand_ID_About = 1,
-  eCommand_ID_Prefs = 2,
-  eCommand_ID_Quit  = 3,
-  eCommand_ID_Last  = 4
+  eCommand_ID_About  = 1,
+  eCommand_ID_Prefs  = 2,
+  eCommand_ID_Quit   = 3,
+  eCommand_ID_Last   = 4,
+  eCommand_ID_Update = 5

Every menuitem has its own command-id, and we start register the regular menu items at eCommand_ID_Last+1.  However,  eCommand_ID_Last+1 is also the value of eCommand_ID_Update. Hence, we registered two commands with the same command id: Check For Updates and New Window.

Otherwise, I'm trying to make this a little bit more reusable for other apps (not the rewrite I would like to see, but still something).

Comment 63

[Asaf Romano (gone)]

Attachment: Small

This is Markus's patch with that bit fixed.

Comment 64

[Asaf Romano (gone)]

Attachment: Big (widget/ part)

Comment 65

[Asaf Romano (gone)]

Attachment: Big (browser/ part)

Comment 66

[Asaf Romano (gone)]

What does Big do? Ugly stuff.

The widget part:
1. Allows adding two custom items after the about item and two custom items after the preferences separator.
========================
= About This App
= Group 1 Item 1       = <- 
= Group 2 Item 2       = <- menu_mac_firstGroup_item_3  (Only checked if item 2 was set)
========================
= Preferences...       = <- menu_preferences
========================
= Group 2 Item 1       = <- menu_mac_secondGroup_item_1  (For exmaple, Clear Recent History)
= Group 2 Item 2       = <- menu_mac_secondGroup_item_2  (Only checked if item 1 was set)
========================
etc.

2. Uses ids in the form of the other app-menu itms: menu_mac_firstGroup_item_2, menu_mac_firstGroup_item_3, menu_mac_secondGroup_item_1, menu_mac_secondGroup_item_2

This means the current system was not rewritten. Instead, I've extended the current mechanism quite a bit.

The browser/ part:
1. Moves Check for Updates to that "first group"
2. Moves Clear Recent History to that "second group"
3. Does not move Private Browsing there.  We cannot do so without rewriting the app-menu system, unless we give up the dynamic label for it as well (or a check-item as implemented by Safari).
4. Add the browser menus to the sanitize window
5. Moves the app-menu xul widgets code out of baseMenuOverlay to browser-*.inc

Comment 67

[Asaf Romano (gone)]

Now, the choice between the two approaches should be done by ux@ and mac-widget peers.  As for my recommendation: as much as I don't like the current system, I think we should go with Big.  For a long time Thunderbird needs custom-items support, at least for its Account Settings item - and so does Firefox (sanitize, private browsing);  Other xul-based applications may also need this feature.

Comment 68

[Asaf Romano (gone)]

Attachment: Check for Updates and Clear Recent History in the Application menu - screenshot

Comment 69

[Mike Beltzner [:beltzner, not reading bugmail]]

UX comment needed on comment 67

Comment 70

[Asaf Romano (gone)]

(In reply to comment #69)
> UX comment needed on comment 67

Indeed.

Comment 71

[Josh Aas]

I think we should take the smaller less-regression-prone patch for Firefox 4 and do the proper rewrite of this system in the next major version of Gecko.

Comment 72

[José Jeria]

Bug 585475 has moved this to the about dialog. See bug 585475, comment 9

Comment 73

[Mike Beltzner [:beltzner, not reading bugmail]]

And as mentioned in that bug, we'll put it in the Application Menu if a patch becomes available.

Comment 74

[Markus Amalthea Magnuson]

(In reply to comment #73)
> And as mentioned in that bug, we'll put it in the Application Menu if a patch
> becomes available.

Uhm, there is a patch available, Mano attached it to this bug in comment 63.

Comment 75

[Stefan [:stefanh]]

So, according to comment #71, I think attachment #463151 [details] [diff] [review] needs ui-review and review.

Comment 76

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 463159 [details]
Check for Updates and Clear Recent History in the Application menu - screenshot

Consider UI review approved, this is where it should be on the Mac.

Comment 77

[Markus Amalthea Magnuson]

So we have ui-review and review now, does it mean check-in?

Comment 78

[Josh Aas]

I think there is a good chance the patch doesn't apply cleanly any more, but when it does I think it is ready for check-in. It does need browser peer review but since Mano is a co-author I think that is probably enough implied approval. Mano can weigh in on that though.

Comment 79

[Markus Amalthea Magnuson]

Attachment: Move "Check for Updates…" to Application menu on Mac

Here's an updated patch that applies to the current tree.

Comment 80

[Josh Aas]

Comment on attachment 471898 [details] [diff] [review]
Move "Check for Updates…" to Application menu on Mac

Looks fine to me, this doesn't need another ui review so I cancelled that. This should be ready for check-in.

Comment 81

[Phil Ringnalda (:philor)]

Comment on attachment 471898 [details] [diff] [review]
Move "Check for Updates…" to Application menu on Mac

Well, it would be ready for check-in if it had approval. Need to land it pretty soon, so that bug 579547 can see how it's going to otherwise break it.

Comment 82

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 471898 [details] [diff] [review]
Move "Check for Updates…" to Application menu on Mac

a=beltzner; a rare approval for a non-blocker, but a much desired piece of polish for OSX!

Comment 83

[Joe Drew (not getting mail)]

Attachment: for check-in

This patch just contains all the metadata needed for easy check-in.

Comment 84

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/071f4fbb4512

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Why do we still have the checkForUpdates entry in the baseMenuOverlay.xul file?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/baseMenuOverlay.xul#112

Shouldn't it be removed?

Comment 86

[Stefan [:stefanh]]

(In reply to comment #85)
> Why do we still have the checkForUpdates entry in the baseMenuOverlay.xul file?
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/baseMenuOverlay.xul#112
> 
> Shouldn't it be removed?

Basically, the mac widget code first hides the menuitem in the help menu, then it creates the same menuitem in the application menu (which is a non-xul menu provided by the OS). It shouldn't really matter where the xul menitem is, but it has to be somewhere in order for the widget code to find it.

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Makes sense. Thanks for the reply. I will continue to use the menu id for our Mozmill tests then.

Otherwise the feature has been landed and can be marked as verified. Tested with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100921 Firefox/4.0b7pre

Comment 88

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Updated the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=10030

Comment 89

[José Jeria]

This option seem to have been removed completely now.

Comment 90

[Phil Ringnalda (:philor)]

From Firefox, because Firefox's UX team chose to do so. The widget code still works just fine.

Comment 91

[Ed Lee :Mardak]

If people were wondering (like me!) this was removed by bug 599480.