263336 - Add keyboard searching of "stored cookies" and "site permissions" lists.

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, enhancement)

Description

[Wevah (Nate Weaver)]

Keyboard searching (either "find as you type"/Finder-style or filtered via a
search field) would be a useful addition to the "stored cookies" and "site
permissions" lists (and other similarly long lists) in Camino's preferences.

Comment 1

[olivier]

Attachment: Includes the modification to the privacy prefPane

The patch modifies the privacy preference pane component to add a SearchField
at the bottom of the cookies permission sheet and at the bottom of the cookies
edition sheet.
The searchTextField also needed to be modified to send out a notification to
the delegate when the text is changed.

Comment 2

[Josh Aas]

Do we really want this? Comments?

Olivier - please attach code patches and new nib files separately.

Comment 3

[Jasper]

As it stands it's relatively easy to find specific cookies because you can sort
the entries using the table headers.

I remember that we discussed this addition on IRC and that we where sceptic
because it was pretty hard not to make it to complicated. Our main concern was
the result list, and how it would be displayed.

But I think that if we placed the search field between the buttons at the bottom
of the sheet. And made sure that the result list work as we wnat and that it's
easy to escape from the result list I wouldn't see any harm in this feature.

So the only way I see this happening is that it's ultra easy to understand and use.

Comment 4

[olivier]

The search box is located in between the done button and the remove button.
Excaping a search is easy, just press the clear button.
AS for locating cookies, whenever i need to change a permission on a cookie, it
is  not made easy by sorting because i have hundred of those, this allow to make
the list much smaller and thus easier to handle (scrolling and all). 
Plus what if you remember the domain name but the cookie host does not start
with it eg: www.internal.host.com. At this point it will take a while to locate it.

Comment 5

[olivier]

Attachment: Archive of all the files modified

Same as patch uploaded, but those are the actual files

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #5)
> Created an attachment (id=167196)
> Archive of all the files modified
> 
> Same as patch uploaded, but those are the actual files

I downloaded the file, is it a gzip, tar, zip file ?

I'll gladly do the review. One more thing you need to be aware of .nib files are
*not* diffeable so If you patch requires a nib files change the procedure is as
follow :
     attach a patch file.
     zip/gzip/stuff the modified nib file and attach it to the bug. Set the
review flag on the patch file (no need to ask it for the nib files - if you
provide a patch)

Comment 7

[olivier]

the file is a zip file, sorry

Comment 8

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)
> Do we really want this? Comments?
> 
Yes. When You have many cookies it can be valuable to have this. I vote for.

Comment 9

[Ludovic Hirlimann [:Usul]]

Comment on attachment 166945 [details] [diff] [review]
Includes the modification to the privacy prefPane

>? camino/PreferencePanes/Privacy/English.lproj/Privacy~.nib

The above line should not be present in the attached patch file. remove it
before attaching patches.

>Index: camino/PreferencePanes/Privacy/PrivacyPane.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/PreferencePanes/Privacy/PrivacyPane.mm,v
>retrieving revision 1.25
>diff -u -F^f -r1.25 PrivacyPane.mm
>--- camino/PreferencePanes/Privacy/PrivacyPane.mm	20 Nov 2004 00:50:51 -0000	1.25
>+++ camino/PreferencePanes/Privacy/PrivacyPane.mm	24 Nov 2004 18:40:00 -0000
>@@ -13,6 +13,7 @@
> #include "nsIURI.h"
> #include "nsNetUtil.h"
> #include "nsString.h"
>+#include "STFPopUpButtonCell.h"
> 
> 
> // prefs for keychain password autofill
>@@ -240,6 +241,17 @@
>   NSPopUpButtonCell *popupButtonCell = [mPermissionColumn dataCell];
>   [popupButtonCell setEditable:YES];
>   [popupButtonCell addItemsWithTitles:[NSArray arrayWithObjects:[self getLocalizedString:@"Allow"], [self getLocalizedString:@"Deny"], nil]];
>+  
>+  //add the menu for the seach field in the cookie panes, steal the strings from the table itself
>+  //
>+  [[(SearchTextFieldCell*)[mCookiesFilterField cell] popUpButtonCell] addItemWithTitle: @"Search all"];

This is not Localizable, you should call getLocalizedString
and should update
mozilla/camino/PreferencePanes/Privacy/English.lproj/Localizable.strings
acordingly. Has for nib files the .strings files are *not* diffeable so it
should ba attached to the bug separatly from the .nib file and the patch.

>+
>+- (void)controlTextDidChange:(NSNotification *)aNotification
>+{
>+	//find out if we are filtering the permission or the cookies
>+	//
>+	NSString *filterString = [[aNotification object] stringValue];
>+		
>+	if (mCachedPermissions && mPermissionManager && ([aNotification object] == mPermissionFilterField)) 
>+	{
>+		//this mean user wants to filter down the list of cookies
>+		//reinitialize the list of permission in case user deleted a letter or replaced a letter
>+		//
>+		[self populatePermissionCache];
>+		
>+		if (![filterString isEqualToString: @""])
>+		{
>+			// remove from parallel array and cookie permissions list
>+			int row = mCachedPermissions->Count() - 1;
>+			for (row ; row>=0 ; row--)
>+			{
>+				nsCAutoString host;
>+				mCachedPermissions->ObjectAt(row)->GetHost(host);
>+				if ([[NSString stringWithUTF8String: host.get()] rangeOfString: filterString].location == NSNotFound)
>+					mCachedPermissions->RemoveObjectAt(row);
>+			}
>+		}
>+		[mPermissionsTable deselectAll: self];   // don't want any traces of previous selection
>+		[mPermissionsTable reloadData];
>+	} 
>+	else if (mCachedCookies && mCookieManager && ([aNotification object] == mCookiesFilterField)) 
>+	{
>+		//this mean user wants to filter down the list of cookies
>+		//reinitialize the list of permission in case user deleted a letter or replaced a letter
>+		//

No need to repeat the comment.

>+		[self populateCookieCache];
>+		
>+		if (![filterString isEqualToString: @""])
>+		{
>+			// remove from parallel array and cookie permissions list
>+			int row = mCachedCookies->Count() - 1;
>+			for (row ; row>=0 ; row--)
>+			{
>+				nsCAutoString host, name, path, cookieVal;
>+				PRUint64 expires = 0;

These variables and "row" should be defined at the function level, not in the
loop.

>+				mCachedCookies->ObjectAt(row)->GetHost(host);
>+				mCachedCookies->ObjectAt(row)->GetName(name);
>+				mCachedCookies->ObjectAt(row)->GetPath(path);
>+				
>+				mCachedCookies->ObjectAt(row)->GetExpires(&expires);
>+				NSString *dateString;
>+				
>+				if (expires == 0) 
>+					// if expires is 0, it's a session cookie; display as expiring on the current date.
>+					// It's not perfect, but it's better than showing the epoch.
>+					dateString = [[[[[mCookiesTable tableColumns] objectAtIndex: 4] dataCell] formatter] stringForObjectValue: [NSDate date]];
>+				else 
>+					dateString = [[[[[mCookiesTable tableColumns] objectAtIndex: 4] dataCell] formatter] stringForObjectValue: [NSDate dateWithTimeIntervalSince1970: (NSTimeInterval)expires]];
>+
>+				mCachedCookies->ObjectAt(row)->GetValue(cookieVal);
>+			
>+				
>+				//see if the user selected something else than the "all" option
>+				//
>+				STFPopUpButtonCell *popupCell = [(SearchTextFieldCell*)[mCookiesFilterField cell] popUpButtonCell];
>+				switch ([popupCell indexOfItem: [popupCell selectedItem]])
>+				{
>+					case 1:
>+						if (([[NSString stringWithUTF8String: host.get()] rangeOfString: filterString].location == NSNotFound) &&
>+							([[NSString stringWithUTF8String: name.get()] rangeOfString: filterString].location == NSNotFound) &&
>+							([[NSString stringWithUTF8String: path.get()] rangeOfString: filterString].location == NSNotFound) &&
>+							([[NSString stringWithUTF8String: cookieVal.get()] rangeOfString: filterString].location == NSNotFound) &&
>+							([dateString rangeOfString: filterString].location == NSNotFound))
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+					case 2:
>+						if ([[NSString stringWithUTF8String: host.get()] rangeOfString: filterString].location == NSNotFound)
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+					case 3:
>+						if ([[NSString stringWithUTF8String: name.get()] rangeOfString: filterString].location == NSNotFound)
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+					case 4:
>+						if ([[NSString stringWithUTF8String: path.get()] rangeOfString: filterString].location == NSNotFound)
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+					case 5:
>+					{
>+						//use the formatter of the column to be sure we have the same format
>+						//
>+						if ([dateString rangeOfString: filterString].location == NSNotFound)
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+					}
>+					case 6:
>+						if ([[NSString stringWithUTF8String: cookieVal.get()] rangeOfString: filterString].location == NSNotFound)
>+							mCachedCookies->RemoveObjectAt(row);
>+						break;
>+				

Could you comment on case numbers what is 1,2, 3, etc ... ?


>+				}
>+			}
>+		}
>+		[mCookiesTable deselectAll: self];   // don't want any traces of previous selection
>+		[mCookiesTable reloadData];
>+	} 
>+}
>+
>+
>+- (void)cookiesSearchFieldCriteriaDidChange:(NSNotification *)aNotification
>+{
>+	NSNotification *newNotification = [NSNotification notificationWithName: [aNotification name] 
>+																	object: [[aNotification object] controlView]];
>+	[self controlTextDidChange: newNotification];
>+}
> @end
>Index: camino/PreferencePanes/Privacy/English.lproj/Privacy.nib/classes.nib
>===================================================================

>Index: camino/PreferencePanes/Privacy/English.lproj/Privacy.nib/info.nib
>===================================================================
>Index: camino/PreferencePanes/Privacy/English.lproj/Privacy.nib/objects.nib
>===================================================================

.nib files need to be attached separetly.

I have not tested the patch completly but it seems to work. I was confused when
I set my first filter because I did not know what criteria where used to filter
- seeing the word "all" in the criteria list at that momment might be a very
good idea setiing r to -. You need to provide use with patch + nib +
Localized.string, before I can do a new review. Also a screenshot would be nice
for people not building wanting to comment on the feature.

Comment 10

[Ludovic Hirlimann [:Usul]]

(In reply to comment #9)
> (From update of attachment 166945 [details] [diff] [review])
> >+- (void)controlTextDidChange:(NSNotification *)aNotification
> >+{
> >+	//find out if we are filtering the permission or the cookies
> >+	//

Also double check your tab settings and change this accordingly to mozilla's
see http://www.mozilla.org/hacking/mozilla-style-guide.html

Comment 11

[olivier]

(In reply to comment #9)


> I set my first filter because I did not know what criteria where used to filter
> - seeing the word "all" in the criteria list at that momment might be a very
> good idea

The string could only be set to this if the search field is not the first
responder. So the problem is that the string can be seen, but then the user has
to click on the field before typing. If we leave the field as first responder,
then the user can start typing right away. Personaly i vote for typing right
away, the learning curve is not that steep, and this would benefit all
subsequent use.

comments?

Comment 12

[olivier]

Attachment: cookie editing pane (pdf file)

Comment 13

[olivier]

Attachment: Cookie editing with menu (pdf file)

Comment 14

[olivier]

Attachment: Cookie permission (pdf file)

Comment 15

[olivier]

Attachment: modified nib file

Comment 16

[olivier]

Attachment: patch file

Comment 17

[Ludovic Hirlimann [:Usul]]

(In reply to comment #16)
> Created an attachment (id=167284)
> patch file
> 

Why did you r+ your own patch ?

Comment 18

[olivier]

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=167284)
> > patch file
> > 
> 
> Why did you r+ your own patch ?

Sorry, i thought plus was for request, then i remember it is ? (by the way why
don't they have full words in there rather than symbol?)

Comment 19

[Josh Aas]

I don't think we should take this patch even if it worked. Seems like bloat to
me. Pinkerton: what do you think? We should decide on this and either push the
patch through or mark this WONTFIX. Obviously I vote the latter. No need for
anyone to spend more time on a bug that won't we don't want to fix.

As Jasper pointed out, you can use column sorting to find things quickly enough.

Comment 20

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

(In reply to comment #19)
> As Jasper pointed out, you can use column sorting to find things quickly enough.

The real-world counter-example to this (at least as far as the site permissions
section goes): I have "Ask before accepting a cookie" set.  Several times
recently I've hit "deny" (and "remember") for a series of cookies from one site,
only to find out that, bah, the site doesn't work without cookies.  There's a
cookie from login.site.com, www.site.com, something.site.com, etc., and I need
to re-enable them all (or at least several of them) to get the site working and
me logged in.

Site permissions sorts (alpha) by first character, so I have to scroll through a
very extensive list looking for all of my site.com permissions to set them to
"allow"--or remove all my allow/deny setting.  Searching, if I understand it
correctly, would let me enter "site.com" and would filter all of my "site.com"
entries.

Similarly, if one wanted to delete all the cookies from site.com in the "Edit
stored cookies" sheet, one currently has to scroll throuhg looking for every
variation.site.com to do so.

If you know every server.site.com, sorting works great; in the real world, the
search would be really handy.

Comment 21

[Mike Pinkerton (not reading bugmail)]

i thought i'd already weighed in here, i think this "feature" proabably
desirable, it's a more advanced panel anyway and can *easily* get swamped with a
huge # of cookies. Making things easier to find is a good thing in this situation.

we should strive to keep it simple, but adding a search field don't overly
complicate it. people are used to this paradigm now. if you totally disagree,
grab me on irc and we can chat about it.

I'd say move it above the list, though, not below it. it should be above, right
justified (like all other search bars in the OS).

Comment 22

[Ludovic Hirlimann [:Usul]]

Comment on attachment 167284 [details] [diff] [review]
patch file

It works. I had this message when I applied the patch : File to patch:
src/browser/SearchTextFieldCell.m
patching file src/browser/SearchTextFieldCell.m
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 262 with fuzz 1. but it built correctly.

Comment 23

[Julian Pellico]

Can someone help me get the new nib to build in the project? I've done all I can
think of and when I build Camino there's no search fields. 

Comment 24

[Ludovic Hirlimann [:Usul]]

(In reply to comment #23)
> Can someone help me get the new nib to build in the project? I've done all I can
> think of and when I build Camino there's no search fields. 

Put it in /Prepanes/Privacy/English.lproj
then delete camino/build/*
build its should work.

Comment 25

[Julian Pellico]

Attachment: picture of window with search field being intrusive

The search field works fine. However, why am I able to resize the sheet so that
the field overlaps Done? I'll have to mark this r-.

Comment 26

[olivier]

Attachment: a new nib file with the sizing of the search field correctly set

Comment 27

[Julian Pellico]

Comment on attachment 168722 [details]
a new nib file with the sizing of the search field correctly set

r+, handing to Ludo

Comment 28

[Ludovic Hirlimann [:Usul]]

Comment on attachment 168722 [details]
a new nib file with the sizing of the search field correctly set

mike .nib file and patch file are ok now, asking for sr and landing.

Comment 29

[Julian Pellico]

(In reply to comment #21)
> I'd say move it above the list, though, not below it. it should be above, right
> justified (like all other search bars in the OS).

Just a note, Apple doesn't _always_ put the search bar in that location. See the
Font panel.

Comment 30

[Mike Pinkerton (not reading bugmail)]

a few things here:

this whole patch has tabs for indents, not spaces. please fix. 2space indents only

+- (void)controlTextDidChange:(NSNotification *)aNotification

why both allowing searching on anything other than host? will users ever need
that kind of flexibility? i mean really, who on earth knows what a cookie's path
is and would search on it?

+ case 1:	//user selected to search in all fielda

each of these cases should be a constant

+  [[NSNotificationCenter defaultCenter] addObserver: self
+                                           selector:
@selector(cookiesSearchFieldCriteriaDidChange:)
+                                               name:
@"STFPopUpButtonCellSelectionChanged"
+                                             object: nil];

do we unregister this observer in dealloc?

+- (void) setMenu: (NSMenu*) inMenu

please comment that this is probably destructive to |inMenu| (it changes the
target/action of each item)

Comment 31

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 168722 [details]
a new nib file with the sizing of the search field correctly set

sr-

Comment 32

[olivier]

Attachment: Modified the source to reflect Mike's comments

Removed the code to search on other field than the host. This simplifies the
code a lot.

Comment 33

[olivier]

Attachment: Modified the source to reflect Mike's comments

Removed the code to search on other field than the host. This simplifies the
code a lot.

Comment 34

[olivier]

Attachment: nib file for the patch

Comment 35

[Ludovic Hirlimann [:Usul]]

(In reply to comment #33)
> Created an attachment (id=170613) [edit]
> Modified the source to reflect Mike's comments

seems ok. 
 
> Removed the code to search on other field than the host. This simplifies the
> code a lot.

The nib file still has the ability to let the user choose the search field and
this "menu" is always empty. Would you care to correct that ?

Comment 36

[olivier]

Attachment: patch file

removed the menu from the search field

Comment 37

[olivier]

Attachment: patch file

removed the menu from the search field plus remove notification for change of
that menu

Comment 38

[Ludovic Hirlimann [:Usul]]

(In reply to comment #37)
> Created an attachment (id=171544) [edit]
> patch file
> 
> removed the menu from the search field plus remove notification for change of
> that menu

Patch does not apply cleanly :
patching file src/browser/SearchTextField.m
Hunk #1 FAILED at 111.
1 out of 1 hunk FAILED -- saving rej

File to patch: PreferencePanes/Privacy/PrivacyPane.h
patching file PreferencePanes/Privacy/PrivacyPane.h
Hunk #2 FAILED at 26.
1 out of 2 hunks FAILED

Would you respining it with an updated source ?

Comment 39

[olivier]

Attachment: source code

this patch should be apply in conjunction with the Attachment #172297 [details] [diff] to Bug
279664 as it depends on the fix for this bug.

Comment 40

[olivier]

Attachment: nib file for the patch (zipped)

Comment 41

[Ludovic Hirlimann [:Usul]]

Comment on attachment 172300 [details] [diff] [review]
source code

asking for second review

Comment 42

[Julian Pellico]

Comment on attachment 172300 [details] [diff] [review]
source code

Just a warning....you may need to re-spin the nib again...I just alerted smfr
to the presence of a bug on that sheet, and he made a change and committed it.

Comment 43

[Julian Pellico]

Comment on attachment 172300 [details] [diff] [review]
source code

Requesting sr from pink. Plase beware the nib issue I mentioned.

Comment 44

[olivier]

Attachment: nib file updated (zipped)

updated as of 1/26/2005 10:40PM easterm time

Comment 45

[Mike Pinkerton (not reading bugmail)]

hold the phone here. there are two search fields? why do you want to search on
the permission field? i'm confused.

Comment 46

[Jasper]

Ha wow I didn't realize it was also for the permssion list. I have a hard time
believing users would ever need that. Cookies overview yes but permissions seems
a bit over the top.

Comment 47

[Wevah (Nate Weaver)]

I dunno, I personally have a few thousand entries in my cookie permissions list
(according to cat hostperm.1 | grep cookie | wc -l , anyway).

Comment 48

[olivier]

(In reply to comment #46)
To me searching the permission is the most important. I set my browser to ask me
before accepting a cooky and i usually deny it. So when i come to a site that
requires it, i have to go back and change the permission. On the other side i
almost never user the cookie overview pane. Different use, different user, i
think both are really useful. Maybe not to the same user though.

Comment 49

[Mike Pinkerton (not reading bugmail)]

I'm really sorry, i thought the two search fields were in the same sheet. i'm a
moron and totally misunderstood what was being searched on for the site
permission field.

i'll re-review in a bit.

Comment 50

[Mike Pinkerton (not reading bugmail)]

a few nits:

+    if (![filterString isEqualToString: @""])

it's faster to do |if ([filterString length])|

+  if (mCachedPermissions && mPermissionManager && ([aNotification object] ==
mPermissionFilterField)) 

i think this is a bit more readable if you put the search field/object
comparison first in each if statement. Thats the most significant condition when
you're scanning down the page.

+      for (row = mCachedPermissions->Count() - 1 ; row>=0 ; row--)

please move the declaration for |row| into the for loop in both loops. no need
to have it at a higher scope if it's only used inside each loop.

+    [self populatePermissionCache];

adding every row back in and then whittling it down again with _every key
stroke_ seems like it would be really slow with a large number of cookies. how
fast is it with a large list? can we do anything to make this better?

Comment 51

[Wevah (Nate Weaver)]

> +    [self populatePermissionCache];
> 
> adding every row back in and then whittling it down again with _every key
> stroke_ seems like it would be really slow with a large number of cookies. how
> fast is it with a large list? can we do anything to make this better?

I like seeing it change on every keystroke. What did Simon do for the
history/bookmark searching? Use a temporary array for the results list? That
seems to be plenty fast.

Comment 52

[Mike Pinkerton (not reading bugmail)]

i agree i like seeing it update live, just wondering if there is a better way to
do it. cc'ing simon in case he has any suggestions, having just done it. can't
hurt to ask the question ;)

Comment 53

[Simon Fraser [no longer active]]

History and bookmarks have a 'search results' mutable array, and on each
keystroke we clear the array, enumerate all the BM/history items, adding
matching ones to the array, then reloadData on the outline view.

This kind of stuff is pretty fast as long as you only update the table view once.

Comment 54

[Mike Pinkerton (not reading bugmail)]

so that's the reverse of what's done here, where everything is added to the list
and what shouldn't be there is stripped away. I guess they even out to the same
complexity, prolly not enough to worry about. If we get reports I guess we can
switch it around later.

sr=pink with the other changes i mentioned above.

Comment 55

[olivier]

Attachment: updated patch file to reflect Mike's comments

Comment 56

[olivier]

Attachment: updated patch to reflect Mike's comment

did a mistake on the previous one, so here is the correct patch

Comment 57

[Mike Pinkerton (not reading bugmail)]

one last nit, i swear next one will get an sr :)

+      int row = 0;
+      for (row = mCachedPermissions->Count() - 1 ; row>=0 ; row--)

should be

for (int row = mCached....)

declare vars in the tightest scope possible.

Comment 58

[olivier]

Attachment: source code patch

Comment 59

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 173173 [details] [diff] [review]
source code patch

sr=pink

Comment 60

[Mike Pinkerton (not reading bugmail)]

latest patch file doesn't apply. i get "patch unexpectedly ends in middle of line"

Comment 61

[Mike Pinkerton (not reading bugmail)]

my bad on the patch, it did apply correctly.

twiddled some of the spacing and comment verbage to be more in line with the
style of the rest of the file.

Oliver, thank you very much for putting up with the review process and for this
contribution. I'm sure folks are gonna love it!! Hope to see more from you in
the future (if you aren't done with us) :)

fixed.