Closed Bug 321937 Opened 19 years ago Closed 14 years ago

Need a disabled "(No Services Found)" menu item when no Bonjour services at start


(Camino Graveyard :: Toolbars & Menus, defect)

Not set


(Not tracked)



(Reporter: hwaara, Assigned: cpeterson)


(Keywords: polish)


(1 file, 2 obsolete files)

Camino 1.0b2

Click on Go > Local Network Services

If there is no service availabe, it looks like this:

About Local Network Services


The separator should not be shown unless there are items.
Assignee: mikepinkerton → nobody
Blocks: 328173
QA Contact: toolbars
Version: Trunk → unspecified
Likewise, if there's no history, we shouldn't have double seperators.  This is a pretty rare case though.
What happens here is that if we have services and then lose them, |availableServicesChanged| gets called, and we append a nice disabled "(No Services Found)" menu item.  If there were never any services to begin with, we just use what we have in the nib (which has the separator).

The easy fix is just to get rid of the separator, which is what comment 0 describes, but the really nice thing would be to have a disabled "(No Services Found)" menu item by default.
(In reply to comment #2)
> the really nice thing would be to have a disabled "(No Services
> Found)" menu item by default.

Sounds good to me.
No longer blocks: 328173
Fixing the summary to reflect comment 2 (since I was changing it because I couldn't find this bug after all the moves/renames).
Summary: No menu separator should be visible in "Local Network Services" menu service is available → Need a disabled "(No Services Found)" menu item when no Bonjour services at start
Chris, you might be interested in this bug, too (desired fix is 2nd pgh of comment 2).
The "Local Network Services" submenu seems to have been removed in Camino 1.5. I don't think this bug is still applicable. :)
It's called "Bonjour" now; the same bug still applies.
Oh, and it's in the Bookmarks Menu now, too; I hadn't realized we had moved and renamed it since the bug was filed.
Attached patch no-services-found-v1.patch (obsolete) — Splinter Review
no-services-found-v1.patch makes the following changes:

1. In setupRendezvous(), add "No Services Found" menu item because we won't know of any Bonjour services until we receive a NetworkServicesAvailableServicesChanged notification.

2. Extract "No Services Found" code to helper function addNoServicesFoundMenuItem().

3. In availableServicesChanged(), consolidate some common code.

4. Remove declaration of unused (and unrelated) function adjustBookmarkMenuItems().

Also, most of the Bonjour code still uses the older "Rendezvous" name. Would you be amenable to a patch that (just) renamed functions and variables named "Rendezvous" with "Bonjour"?
Attachment #471768 - Flags: review?(stuart.morgan+bugzilla)
Assignee: nobody →
Comment on attachment 471768 [details] [diff] [review]

Looks good, r=me.

Two nits (both about existing code, but as long as you are touching it anyway...):

>+  NSMenuItem* newItem = [mServersSubmenu addItemWithTitle:NSLocalizedString(@"NoServicesFound", @"") action:nil keyEquivalent:@""];

Break up the long line a bit:

NSMenuItem* newItem = [mServersSubmenu addItemWithTitle:NSLocalizedString(@"NoServicesFound", @"")

>+  // add a separator
>+  [mServersSubmenu addItem:[NSMenuItem separatorItem]];

Remove the useless comment.
Attachment #471768 - Flags: review?(stuart.morgan+bugzilla) → review+
Attached patch no-services-found-v2.patch (obsolete) — Splinter Review
Incorporate smorgan's feedback.
Attachment #471768 - Attachment is obsolete: true
Attachment #471942 - Flags: review?(cl-bugs-new2)
Comment on attachment 471942 [details] [diff] [review]

In general, once you've addressed the review comments, there's no need to ask for a second review unless you just want the original reviewer to sanity-check it. :)

I'll pass this on to pink for sr.
Attachment #471942 - Flags: review?(cl-bugs-new2) → superreview?(mikepinkerton)
Our docs still say we do two reviews before sr, but we mostly just do one now because of manpower limits.
Specifically, that bit remains in the docs so that we can continue to adhere to it in certain situations without taking anyone by surprise.  We do usually follow what's stated there for an initial period with new contributors--though that works better when there are large or complex patches that benefit from two sets of eyes (the kinds of patches where we might still seek two reviews before sr from existing contributors).

For the well-scoped and non-Gecko-related patches you've been writing, I don't think there's any need to seek a second review before sr unless your first reviewer requests it :)
Comment on attachment 471942 [details] [diff] [review]

+  [newItem setTag:-1];

comment on why you're setting the tag to -1?

Attachment #471942 - Flags: superreview?(mikepinkerton) → superreview+
Good question. I did not originally write that block of code. I extracted it from the method availableServicesChanged into a new method addNoServicesFoundMenuItem to avoid code duplication. My patch calls addNoServicesFoundMenuItem from both availableServicesChanged (preserving previous behavior) and setupRendezvous (for proper menu init).

I can remove setTag if it is inappropriate..?
That line dates to smfr's initial implementation: (bug 176348), which, of course, is explained in great detail, as with most old Camino checkins :P

How do we get rid of that item when Services reappear?  Could the tag be for that?  (When we figure it out, and if it's still needed, it needs a code comment for exactly this reason ;) )
no-services-found-v3.patch removes [newItem setTag:-1]. I have tested that the tag is not necessary for updating the menu when Bonjour services come and go. The menu is updated when the NetworkServicesAvailableServicesChanged notification calls our availableServicesChanged callback.
Attachment #471942 - Attachment is obsolete: true
Keywords: checkin-needed

Thanks, Chris!

(In reply to comment #19)
> checkin-needed

You don't really need to do that unless I'm on vacation or otherwise missing, or if you've put the final for-checkin patch up and it's been a few days and for some reason no-one's landed it (we somehow missed it) ;)  I keep an eye on changed bugs, and the extra checkin-needed keyword tends to end up being something else I forget to change when resolving the bug ;)
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.