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

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Håkan Waara]

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.

Comment 1

[froodian (Ian Leue)]

Likewise, if there's no history, we shouldn't have double seperators.  This is a pretty rare case though.

Comment 2

[froodian (Ian Leue)]

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.

Comment 3

[Håkan Waara]

(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.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Fixing the summary to reflect comment 2 (since I was changing it because I couldn't find this bug after all the moves/renames).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Chris, you might be interested in this bug, too (desired fix is 2nd pgh of comment 2).

Comment 6

[Chris Peterson [:cpeterson]]

The "Local Network Services" submenu seems to have been removed in Camino 1.5. I don't think this bug is still applicable. :)

Comment 7

[Stuart Morgan]

It's called "Bonjour" now; the same bug still applies.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: no-services-found-v1.patch

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"?

Comment 10

[Stuart Morgan]

Comment on attachment 471768 [details] [diff] [review]
no-services-found-v1.patch

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", @"")
                                                 action:nil
                                          keyEquivalent:@""];

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

Remove the useless comment.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: no-services-found-v2.patch

Incorporate smorgan's feedback.

Comment 12

[Chris Lawson (gone)]

Comment on attachment 471942 [details] [diff] [review]
no-services-found-v2.patch

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.

Comment 13

[Stuart Morgan]

Our docs still say we do two reviews before sr, but we mostly just do one now because of manpower limits.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 471942 [details] [diff] [review]
no-services-found-v2.patch

+  [newItem setTag:-1];

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

sr=pink

Comment 16

[Chris Peterson [:cpeterson]]

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..?

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

That line dates to smfr's initial implementation: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/camino/src/application/MainController.mm&rev=CHIMERA_M1_0_1_BRANCH&mark=2207#2207 (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 ;) )

Comment 18

[Chris Peterson [:cpeterson]]

Attachment: no-services-found-v3.patch

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.

Comment 19

[Chris Peterson [:cpeterson]]

checkin-needed

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/b33ae2cc4e54

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 ;)