Closed Bug 332542 Opened 18 years ago Closed 18 years ago

Bookmarking a tab group uses frontmost page title, confusing users

Categories

(Camino Graveyard :: Bookmarks, defect)

PowerPC
macOS
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla-graveyard, Assigned: bugzilla-graveyard)

References

Details

(Keywords: fixed1.8.1, polish)

Attachments

(2 files, 6 obsolete files)

When bookmarking multiple tabs, we still use the page title of the frontmost tab for the group title. This is, at best, a little confusing, and at worst, wrong.

If we decide this should be fixed, I'll take it. I suspect most users would be renaming the tab group to something meaningful to them, so I'll put forth the suggestion that when "Bookmark All Tabs" is checked, we should rewrite the title field to "[tab group]" or something similar.
I use them three ways:

Very rarely, a page and its subsidiary links (say, an RFC and the things it references), where "Page title" makes some sense,

Fairly often, a cheap Session Saver, where "[tab group]" (that I may or may not add a date onto) works great, and stands out more than the title of one page I had open would,

When I'm setting up a new profile or browser, sets of things I use for a particular purpose (tinderbox, bonsai, a bug query; RSS aggregator, Reddit, weblog posting form), where one page title makes no sense.

Since even "[tab group] First Page Title" will still look like something the user ought to leave that way, I'd say you're right, change it to something dull and always the same, to encourage changing while still standing out when it was just a quick temporary save.
How about "[n tabs] title of active page" ?

When I'm in a hurry (which is a lot of times, using the group as a quick session-saver/file-for-later), I'd much rather have some idea of what the group is when I come back to it later (as well as some additional info that it's a group, not a folder or single bookmark).

I think my proposal is better than a completely generic "[tab group]" label--a lot of people may never rename them, even if they should, so having 10 different "[tab group]" items is bad ;)--and it's a slightly more descriptive compromise than "[tab group] title of active page".  

It's not perfect, but it would make me happier--and we know Camino development is all about making me happy ;)
I vote for Smokey's suggestion in comment 2.
Attached patch first shot at a patch (obsolete) — Splinter Review
Assignee: mikepinkerton → bugzilla
Status: NEW → ASSIGNED
Attachment #221130 - Flags: review?(stuart.morgan)
Comment on attachment 221130 [details] [diff] [review]
first shot at a patch

>+- (IBAction)toggleTabGroupTitlePrefix:(id)sender
>+{
>+  BOOL titleWasEdited = !(([[mTitleField stringValue] isEqualToString:mTitleString]) ||
>+                         ([[mTitleField stringValue] isEqualToString:[NSString stringWithFormat:@"[%d tabs] %@", [mBookmarkItems count], mTitleString]]));

You can remove at least one pair of parantheses there. Also (as mentioned on IRC), this could be done by being the textfield's delegate, and just skipping all editing code if the user has touched the string, but this approach looks okay too.

>+  if (titleWasEdited) { // user edited the bookmark title, so don't change it
>+    NSLog(@"Title was edited; leaving title alone.");
>+    return;
>+  }

Please remove the NSLog.

>+  else { // prepend "[n tabs] " to the title if it's a tab group or remove prefix if box is unchecked
>+    NSLog(@"Title hasn't been edited; auto-changing title.");
>+    NSString* title = @"";
>+    if ([mTabGroupCheckbox state] == NSOnState)
>+      title = [NSString stringWithFormat:@"[%d tabs] %@", [mBookmarkItems count], mTitleString];
>+    else
>+      title = [NSString stringWithString:mTitleString];
>+
>+    [mTitleField setStringValue:title];
>+  }

* No need for an else clause after the early return case. 
* Please remove the NSLog :-)
* No need to init the title if you're gonna change it immediately and unconditionally.
* Instead of |title = [NSString stringWithString:mTitleString];|, how about simply |title = mTitleString;|?


>+}
>+
>+- (void)setTitleString:(NSString*)aString
>+{
>+  [mTitleString autorelease];
>+  mTitleString = [aString copy];
>+}

The copy is generally a nice design pattern if you might be getting a mutable string (the copy will guarantee you get a immutable one). In this case, the only caller gives a static string, so the copy is not really needed and we could as well just retain.
Attached patch addresses Håkan's comments (obsolete) — Splinter Review
I've addressed all the comments except for the copy; I'm leaving the copy in there in case someone in the future decides to call the setter with a mutable string. (It's not hurting anything the way it is, right?)

cl
Attachment #221130 - Attachment is obsolete: true
Attachment #221132 - Flags: review?(stuart.morgan)
Attachment #221130 - Flags: review?(stuart.morgan)
Attached file nib changes (obsolete) —
Attachment #221134 - Flags: review?(stuart.morgan)
Comment on attachment 221132 [details] [diff] [review]
addresses Håkan's comments

>+  NSString*                 mTitleString;
...
>+- (void)setTitleString:(NSString*)aString;

What you are storing isn't the title string, it's the default title string; the name of the variable and the setter should reflect that. And this should be in a private category, not the header.

+- (IBAction)toggleTabGroupTitlePrefix:(id)sender;

Just call this toggleTabGroup:.  That's what is actually being done, it just happens that the title is all that's changing at the moment.

>+  BOOL titleWasEdited = !([[mTitleField stringValue] isEqualToString:mTitleString] ||
>+                          [[mTitleField stringValue] isEqualToString:[NSString stringWithFormat:@"[%d tabs] %@", [mBookmarkItems count], mTitleString]]);
>+
>+  if (titleWasEdited) // user edited the bookmark title, so don't change it

No need to store a value that is only used on the next line; just make that the if condition.

>+// prepend "[n tabs] " to the title if it's a tab group or remove prefix if box is unchecked
>+  NSString* title;
>+  if ([mTabGroupCheckbox state] == NSOnState)
>+    title = [NSString stringWithFormat:@"[%d tabs] %@", [mBookmarkItems count], mTitleString];
>+  else
>+    title = mTitleString;

Again, no need for the intermediate value, and the tab prefix needs to be localizable. And since you need that string value at least once on every call, and sometimes twice, it would be cleaner to store it as a local variable right away, and use that in the if and the setting.

And since at that makes the value to set short, just use a ternary (i.e., [mTitleField setStringValue:([mTabGroupCheckbox state] == NSOnState) ? mDefaultGroupTitle : mDefaultTitle];)

Also, fix the spacing on the comment.

>+- (void)setTitleString:(NSString*)aString
>+{
>+  [mTitleString autorelease];
>+  mTitleString = [aString copy];
>+}

Just use retain; it's a private function, so you don't have to worry about unforseen input.

>-
>+  

Don't add whitespace to blank lines


As a general note: why a prefix instead of a suffix?  I would think the title would be more important than the number of tabs.
Attachment #221132 - Flags: review?(stuart.morgan) → review-
Attached patch new patch addressing comments (obsolete) — Splinter Review
Attachment #221132 - Attachment is obsolete: true
Attachment #221138 - Flags: review?(stuart.morgan)
Attached file new nib changes
Attachment #221134 - Attachment is obsolete: true
Attachment #221139 - Flags: review?(stuart.morgan)
Attachment #221134 - Flags: review?(stuart.morgan)
If a page's title is really long, you wouldn't notice a suffix, so I think a prefix could work better.

r=me on the latest patch, fwiw.
Attachment #221139 - Flags: review?(stuart.morgan) → review+
Comment on attachment 221138 [details] [diff] [review]
new patch addressing comments

This works and looks good, except for:

[NSString stringWithFormat:@"[%d %@] %@", [mBookmarkItems count], NSLocalizedString(@"tabs", @""), mDefaultTitle]

That leaves the overall format fixed, which could potentially be a problem for localizers.  The format string itself  should be localized ("[%d tabs] %@") instead. (Be sure to put the English version in the comment argument of NSLocalizedString to make it clear what the arguments are for.)
Attachment #221138 - Flags: review?(stuart.morgan) → review-
Attached patch updated patch for l10n (obsolete) — Splinter Review
I suspect the nib is obsolete now, too. I'll post a new one.

cl
Attachment #221138 - Attachment is obsolete: true
Attachment #221139 - Attachment is obsolete: true
Attachment #223580 - Flags: review?(stuart.morgan)
Comment on attachment 221139 [details]
new nib changes

I lied. This isn't obsolete at all. Sorry for the bugspam.
Attachment #221139 - Attachment is obsolete: false
Comment on attachment 223580 [details] [diff] [review]
updated patch for l10n

>+  NSString* defaultGroupTitle = [NSString stringWithFormat:NSLocalizedString(@"[%d tabs] %@", @"[n tabs] title"), [mBookmarkItems count], mDefaultTitle];

Using the formatter as the key is pretty icky--just give this a key that makes it clear what it is, like defaultTabGroupTitle. (That's why I suggested putting the formater as the comment; the key won't make it clear what the arguments are for.)

Also (sorry for missing this before), instead of:

if (!foo)
  return;
do things

just make it:
if (foo)
  do things

>+  // prepend "[n tabs] " to the title if it's a tab group or remove prefix if box is unchecked

This comment is fragile to formatter changes; keep it more generic. In combination with the above, something like:

// if the title is unedited, update it based on the tab group setting
if (foo)
  ...
Attachment #223580 - Flags: review?(stuart.morgan) → review-
*** Bug 304035 has been marked as a duplicate of this bug. ***
Attached patch last one, I hope (obsolete) — Splinter Review
This should do it, I think.
Attachment #223580 - Attachment is obsolete: true
Attachment #229258 - Flags: review?(stuart.morgan)
Oh, and a note to whomever checks this in: there will need to be a Localized.strings change that goes along with it.

cl
Comment on attachment 229258 [details] [diff] [review]
last one, I hope

>+  if (([[mTitleField stringValue] isEqualToString:mDefaultTitle] ||
>+       [[mTitleField stringValue] isEqualToString:defaultGroupTitle]))
>+    {
>+      [mTitleField setStringValue:([mTabGroupCheckbox state] == NSOnState) ? defaultGroupTitle : mDefaultTitle];
>+    }

Get rid of the double parens, and shift the braces and their contents left.

>-
>+  [self setDefaultTitle:@""];
>+  

Don't add whitespace to blank lines

r=me with those changes.
Attachment #229258 - Flags: review?(stuart.morgan) → review+
Attached patch r=smorgan patchSplinter Review
No reason to hold this up over trivial changes while cl is MIA.  Makes smorgan's changes.  cl, hope you don't mind.
Attachment #229258 - Attachment is obsolete: true
Attachment #231316 - Flags: superreview?(mikepinkerton)
Comment on attachment 231316 [details] [diff] [review]
r=smorgan patch

sr=pink
Attachment #231316 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]
(In reply to comment #18)
> Oh, and a note to whomever checks this in: there will need to be a
> Localized.strings change that goes along with it.

Chris, what is the localizable.string that needs to be added?
Whiteboard: [needs checkin] → [needs checkin][needs strings change comment ???]
From the patch:

"defaultTabGroupTitle"

cl
/* Used when bookmarking multiple tabs; most likely you will only need to localize the word tabs and the order of %d and tabs. */
"defaultTabGroupTitle" = "[%d tabs] %@";


Like that, then?  Or are they only localizing "tabs"?
Whiteboard: [needs checkin][needs strings change comment ???] → [needs checkin][needs strings change comment 24]
(In reply to comment #24)
> "defaultTabGroupTitle" = "[%d tabs] %@";
> 
> Like that, then?

Yes
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You missed the strings change again; the string (and its prefatory comment) are in comment 24, as the status whiteboard indicates.
(In reply to comment #27)
> You missed the strings change again; the string (and its prefatory comment) are
> in comment 24, as the status whiteboard indicates.
> 

Fixed trunk and branch, thanks for the catch smokey.
Whiteboard: [needs checkin][needs strings change comment 24]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: