Closed Bug 306018 Opened 15 years ago Closed 9 years ago

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

Categories

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

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: beltzner, Assigned: markus.magnuson)

References

Details

(Keywords: polish, Whiteboard: design in comment 12)

Attachments

(5 files, 5 obsolete files)

22.63 KB, patch
Details | Diff | Splinter Review
10.47 KB, patch
Details | Diff | Splinter Review
29.71 KB, image/png
Details
10.36 KB, patch
jaas
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
10.58 KB, patch
Details | Diff | Splinter Review
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.
Flags: blocking1.8b4?
We can do a band-aid here (and for "Clear Private Data" too). However, I'm a bit
questioning the value of it.
Flags: blocking1.8b4? → blocking1.8b4+
Assignee: nobody → joshmoz
taking per IRC.
Assignee: joshmoz → bugs.mano
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Firefox1.5
Attached patch patch (obsolete) — Splinter Review
Moves the update menu item from the 'Help' menu to the app menu.
Attachment #194252 - Flags: review?(bugs.mano)
Are you sure about moving "Clear Private Data" also?  Can't it just stay in 'Tools'?
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.
Attachment #194252 - Flags: review?(bugs.mano) → review+
(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 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.
Attachment #194252 - Flags: review+ → review-
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.
(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?
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".
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
Flags: blocking1.8b5+ → blocking1.8b5-
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.
(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.
(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.
Duplicate of this bug: 431410
Target Milestone: Firefox1.5 → ---
Version: 1.5.0.x Branch → Trunk
Duplicate of this bug: 434218
Duplicate of this bug: 440631
Product: Firefox → Toolkit
Moving over to Firefox -> Menus since that is what needs to be modified.
Component: Application Update → Menus
Product: Toolkit → Firefox
QA Contact: application.update → menus
Hardware: PowerPC → All
Joe said he was interested in taking this ...
Duplicate of this bug: 501293
Summary: [ui polish] Move "Check for Updates" into application menu on OSX → [ui polish] Move "Check for Updates" into Firefox menu on OSX
It is been 4 years?

Firefox 3.5 is out.

What a joke.
Mano, you wanna still work on this bug? Otherwise we should leave it for another person.
Flags: blocking-firefox3.6?
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.
Assignee: mano → nobody
Status: ASSIGNED → NEW
Flags: blocking-firefox3.6? → blocking-firefox3.6-
Whiteboard: SEE COMMENT 8 → design in comment 12
Keywords: polish
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.
Duplicate of this bug: 538130
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?
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.
Assignee: nobody → markus.magnuson
Attachment #194252 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #421625 - Flags: ui-review?(faaborg)
Attachment #421625 - Flags: review?(mano)
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
I want to review this patch before it goes in, request when you're ready.
(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.
Attachment #421625 - Flags: ui-review?(faaborg) → ui-review+
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.
Attachment #421625 - Flags: review?(mano) → review-
I agree with Mano in the long run but I don't think it needs to hold this up.
(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?
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 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.
Attachment #421625 - Flags: review-
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?
Attachment #421625 - Attachment is obsolete: true
Attachment #422616 - Flags: review?(joshmoz)
Uhm, as mentioned in comment 36, that is.
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.
(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?
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).
(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.
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!
(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.
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.
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.
(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.
Attachment #422616 - Flags: review?(joshmoz)
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.
Attachment #422616 - Attachment is obsolete: true
Attachment #429528 - Flags: review?(joshmoz)
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.
Attachment #429528 - Flags: review?(joshmoz) → review?(mano)
So, nothing's gonna happen with this patch? Should I spend any more energy on it?
I'll look into this next week.  I cannot promise that I know checkForUpdates better than josh though.
Will this patch make it into Firefox 4? The review has been pending for 4 months now.
Well, depends if the design direction is now moving it to about:Dialog
(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.
(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.
Patch still applies, strange window behavior still present.
Duplicate of this bug: 578944
Markus: since Mano isn't answering, perhaps you should ask for Josh's review instead?
Component: Menus → Widget: Cocoa
Flags: blocking-firefox3.6-
Product: Firefox → Core
QA Contact: menus → cocoa
Summary: [ui polish] Move "Check for Updates" into Firefox menu on OSX → [ui polish] Move "Check for Updates" into Application menu on OSX
Duplicate of this bug: 578945
Attachment #429528 - Attachment description: Move the software update item from Help menu to application menu on Mac. → Move the software update item from Help menu to application menu on Mac.
Attachment #429528 - Flags: review?(mano) → review?(joshmoz)
Attachment #429528 - Flags: review?(joshmoz)
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.
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).
Attached patch Small (obsolete) — Splinter Review
This is Markus's patch with that bit fixed.
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
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.
UX comment needed on comment 67
Keywords: uiwanted
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.
Bug 585475 has moved this to the about dialog. See bug 585475, comment 9
And as mentioned in that bug, we'll put it in the Application Menu if a patch becomes available.
(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.
Flags: blocking1.8b5-
So, according to comment #71, I think attachment #463151 [details] [diff] [review] needs ui-review and review.
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.
Attachment #463159 - Flags: ui-review+
Attachment #463151 - Flags: review+
So we have ui-review and review now, does it mean check-in?
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.
Here's an updated patch that applies to the current tree.
Attachment #429528 - Attachment is obsolete: true
Attachment #463151 - Attachment is obsolete: true
Attachment #471898 - Flags: ui-review?(limi)
Attachment #471898 - Flags: review?(joshmoz)
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.
Attachment #471898 - Flags: ui-review?(limi)
Attachment #471898 - Flags: review?(joshmoz)
Attachment #471898 - Flags: review+
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.
Attachment #471898 - Flags: approval2.0?
Keywords: uiwanted
Blocks: 579547
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!
Attachment #471898 - Flags: approval2.0? → approval2.0+
Flags: in-litmus?
Attached patch for check-inSplinter Review
This patch just contains all the metadata needed for easy check-in.
http://hg.mozilla.org/mozilla-central/rev/071f4fbb4512
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
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?
(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.
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
Status: RESOLVED → VERIFIED
Updated the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=10030
Flags: in-litmus? → in-litmus+
This option seem to have been removed completely now.
From Firefox, because Firefox's UX team chose to do so. The widget code still works just fine.
If people were wondering (like me!) this was removed by bug 599480.
Blocks: 599480
You need to log in before you can comment on or make changes to this bug.