Bookmarking a tab group uses frontmost page title, confusing users

RESOLVED FIXED

Status

Camino Graveyard
Bookmarks
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Chris Lawson (gone), Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1, polish})

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

12 years ago
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 ;)

Comment 3

12 years ago
I vote for Smokey's suggestion in comment 2.
(Assignee)

Comment 4

12 years ago
Created attachment 221130 [details] [diff] [review]
first shot at a patch
Assignee: mikepinkerton → bugzilla
Status: NEW → ASSIGNED
Attachment #221130 - Flags: review?(stuart.morgan)

Comment 5

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

Comment 6

12 years ago
Created attachment 221132 [details] [diff] [review]
addresses Håkan's comments

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)
(Assignee)

Comment 7

12 years ago
Created attachment 221134 [details]
nib changes
Attachment #221134 - Flags: review?(stuart.morgan)

Comment 8

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

Comment 9

12 years ago
Created attachment 221138 [details] [diff] [review]
new patch addressing comments
Attachment #221132 - Attachment is obsolete: true
Attachment #221138 - Flags: review?(stuart.morgan)
(Assignee)

Comment 10

12 years ago
Created attachment 221139 [details]
new nib changes
Attachment #221134 - Attachment is obsolete: true
Attachment #221139 - Flags: review?(stuart.morgan)
Attachment #221134 - Flags: review?(stuart.morgan)

Comment 11

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

Updated

11 years ago
Attachment #221139 - Flags: review?(stuart.morgan) → review+

Comment 12

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

Comment 13

11 years ago
Created attachment 223580 [details] [diff] [review]
updated patch for l10n

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)
(Assignee)

Comment 14

11 years ago
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 15

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

Comment 16

11 years ago
*** Bug 304035 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

11 years ago
Created attachment 229258 [details] [diff] [review]
last one, I hope

This should do it, I think.
Attachment #223580 - Attachment is obsolete: true
Attachment #229258 - Flags: review?(stuart.morgan)
(Assignee)

Comment 18

11 years ago
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 19

11 years ago
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+

Comment 20

11 years ago
Created attachment 231316 [details] [diff] [review]
r=smorgan patch

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+

Updated

11 years ago
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 ???]
(Assignee)

Comment 23

11 years ago
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]

Comment 25

11 years ago
(In reply to comment #24)
> "defaultTabGroupTitle" = "[%d tabs] %@";
> 
> Like that, then?

Yes

Comment 26

11 years ago
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1
You missed the strings change again; the string (and its prefatory comment) are in comment 24, as the status whiteboard indicates.

Comment 28

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