All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

--
trivial
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: hwaara, Assigned: cpeterson)

Tracking

({polish})

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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

Comment 1

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

Comment 2

12 years ago
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.
(Reporter)

Comment 3

12 years ago
(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.

Updated

12 years ago
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).
(Assignee)

Comment 6

8 years ago
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

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Created attachment 471768 [details] [diff] [review]
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"?
Attachment #471768 - Flags: review?(stuart.morgan+bugzilla)
Assignee: nobody → mozilla.org
Status: NEW → ASSIGNED

Comment 10

8 years ago
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.
Attachment #471768 - Flags: review?(stuart.morgan+bugzilla) → review+
(Assignee)

Comment 11

8 years ago
Created attachment 471942 [details] [diff] [review]
no-services-found-v2.patch

Incorporate smorgan's feedback.
Attachment #471768 - Attachment is obsolete: true
Attachment #471942 - Flags: review?(cl-bugs-new2)

Comment 12

8 years ago
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.
Attachment #471942 - Flags: review?(cl-bugs-new2) → superreview?(mikepinkerton)

Comment 13

8 years ago
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]
no-services-found-v2.patch

+  [newItem setTag:-1];

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

sr=pink
Attachment #471942 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 16

8 years ago
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: 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 ;) )
(Assignee)

Comment 18

8 years ago
Created attachment 476698 [details] [diff] [review]
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.
Attachment #471942 - Attachment is obsolete: true
(Assignee)

Comment 19

8 years ago
checkin-needed
Keywords: checkin-needed
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 ;)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.