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

RESOLVED FIXED in Camino0.9

Status

Camino Graveyard
Preferences
--
enhancement
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Wevah, Assigned: Mike Pinkerton (not reading bugmail))

Tracking

unspecified
Camino0.9
PowerPC
Mac OS X

Details

Attachments

(6 attachments, 14 obsolete attachments)

48.06 KB, application/pdf
Details
84.42 KB, application/pdf
Details
19.61 KB, application/pdf
Details
141.77 KB, image/tiff
Details
6.22 KB, application/zip
Details
4.88 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

14 years ago
Target Milestone: --- → Camino0.9

Comment 1

14 years ago
Created attachment 166945 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #166945 - Flags: review?(pinkerton)

Comment 2

14 years ago
Do we really want this? Comments?

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

Comment 3

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

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

14 years ago
Created attachment 167196 [details]
Archive of all the files modified

Same as patch uploaded, but those are the actual files

Updated

14 years ago
Attachment #166945 - Flags: review?(pinkerton) → review?(qa-mozilla)
(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

14 years ago
the file is a zip file, sorry
(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 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.
Attachment #166945 - Flags: review?(qa-mozilla) → review-
Attachment #166945 - Attachment is obsolete: true
Attachment #167196 - Attachment is obsolete: true
(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

14 years ago
(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

14 years ago
Created attachment 167277 [details]
cookie editing pane (pdf file)

Comment 13

14 years ago
Created attachment 167278 [details]
Cookie editing with menu (pdf file)

Comment 14

14 years ago
Created attachment 167280 [details]
Cookie permission (pdf file)

Comment 15

14 years ago
Created attachment 167281 [details]
modified nib file

Comment 16

14 years ago
Created attachment 167284 [details] [diff] [review]
patch file

Updated

14 years ago
Attachment #167284 - Flags: review+
(In reply to comment #16)
> Created an attachment (id=167284)
> patch file
> 

Why did you r+ your own patch ?

Updated

14 years ago
Attachment #167284 - Flags: review+ → review?

Updated

14 years ago
Attachment #167284 - Flags: review? → review?(qa-mozilla)

Comment 18

14 years ago
(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

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

Comment 21

14 years ago
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 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.
Attachment #167284 - Flags: review?(qa-mozilla)
Attachment #167284 - Flags: review?(jpellico)
Attachment #167284 - Flags: review+

Comment 23

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

14 years ago
Created attachment 168394 [details]
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-.

Updated

14 years ago
Attachment #167284 - Flags: review?(jpellico) → review-

Comment 26

14 years ago
Created attachment 168722 [details]
a new nib file with the sizing of the search field correctly set
Attachment #167281 - Attachment is obsolete: true

Updated

14 years ago
Attachment #168722 - Flags: review?(jpellico)

Comment 27

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

r+, handing to Ludo
Attachment #168722 - Flags: review?(qa-mozilla)
Attachment #168722 - Flags: review?(jpellico)
Attachment #168722 - Flags: review+
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.
Attachment #168722 - Flags: superreview?(pinkerton)
Attachment #168722 - Flags: review?(qa-mozilla)
Attachment #168722 - Flags: review+

Comment 29

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

Comment 30

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

Comment 31

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

sr-
Attachment #168722 - Flags: superreview?(pinkerton) → superreview-

Comment 32

14 years ago
Created attachment 170612 [details] [diff] [review]
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.
Attachment #170612 - Flags: review?(qa-mozilla)

Comment 33

14 years ago
Created attachment 170613 [details] [diff] [review]
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.
Attachment #167284 - Attachment is obsolete: true
Attachment #168722 - Attachment is obsolete: true
Attachment #170613 - Flags: review?(qa-mozilla)

Comment 34

14 years ago
Created attachment 170615 [details]
nib file for the patch
Attachment #170615 - Flags: review?(qa-mozilla)
(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

14 years ago
Created attachment 171541 [details] [diff] [review]
patch file

removed the menu from the search field
Attachment #170612 - Attachment is obsolete: true
Attachment #170613 - Attachment is obsolete: true
Attachment #171541 - Flags: review?(qa-mozilla)

Comment 37

14 years ago
Created attachment 171544 [details] [diff] [review]
patch file

removed the menu from the search field plus remove notification for change of
that menu
Attachment #171541 - Attachment is obsolete: true
Attachment #171544 - Flags: review?(qa-mozilla)
(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

14 years ago
Created attachment 172300 [details] [diff] [review]
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.
Attachment #171544 - Attachment is obsolete: true
Attachment #172300 - Flags: review?(qa-mozilla)

Comment 40

14 years ago
Created attachment 172301 [details]
nib file for the patch (zipped)
Attachment #170615 - Attachment is obsolete: true

Updated

14 years ago
Depends on: 279664
Comment on attachment 172300 [details] [diff] [review]
source code

asking for second review
Attachment #172300 - Flags: review?(qa-mozilla)
Attachment #172300 - Flags: review?(jpellico)
Attachment #172300 - Flags: review+
Attachment #170612 - Flags: review?(qa-mozilla)
Attachment #170613 - Flags: review?(qa-mozilla)
Attachment #170615 - Flags: review?(qa-mozilla)
Attachment #171541 - Flags: review?(qa-mozilla)
Attachment #171544 - Flags: review?(qa-mozilla)

Comment 42

14 years ago
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.
Attachment #172300 - Flags: review?(jpellico) → review+

Comment 43

14 years ago
Comment on attachment 172300 [details] [diff] [review]
source code

Requesting sr from pink. Plase beware the nib issue I mentioned.
Attachment #172300 - Flags: superreview?(pinkerton)

Comment 44

14 years ago
Created attachment 172518 [details]
nib file updated (zipped)

updated as of 1/26/2005 10:40PM easterm time
Attachment #172301 - Attachment is obsolete: true
(Assignee)

Comment 45

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

Comment 46

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

Comment 47

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

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

Comment 49

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

Comment 50

14 years ago
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?
(Reporter)

Comment 51

14 years ago
> +    [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.
(Assignee)

Comment 52

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

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

Comment 54

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

14 years ago
Created attachment 173080 [details] [diff] [review]
updated patch file to reflect Mike's comments

Updated

14 years ago
Attachment #172300 - Attachment is obsolete: true
Attachment #173080 - Flags: superreview?(pinkerton)

Comment 56

14 years ago
Created attachment 173081 [details] [diff] [review]
updated patch to reflect Mike's comment

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

Updated

14 years ago
Attachment #173080 - Attachment is obsolete: true

Updated

14 years ago
Attachment #173081 - Flags: superreview?(pinkerton)
(Assignee)

Comment 57

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

14 years ago
Created attachment 173173 [details] [diff] [review]
source code patch
Attachment #173081 - Attachment is obsolete: true
Attachment #173173 - Flags: superreview?(pinkerton)
(Assignee)

Comment 59

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

sr=pink
Attachment #173173 - Flags: superreview?(pinkerton) → superreview+
(Assignee)

Updated

14 years ago
Attachment #173081 - Flags: superreview?(pinkerton)
(Assignee)

Updated

14 years ago
Attachment #173080 - Flags: superreview?(pinkerton)
(Assignee)

Comment 60

14 years ago
latest patch file doesn't apply. i get "patch unexpectedly ends in middle of line"
(Assignee)

Comment 61

14 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Attachment #172300 - Flags: superreview?(pinkerton)
You need to log in before you can comment on or make changes to this bug.