Closed Bug 297360 Opened 19 years ago Closed 19 years ago

doubled items in saved search when adding new items above some old items

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird1.1

People

(Reporter: asa, Assigned: mscott)

References

Details

Attachments

(3 files, 2 obsolete files)

If you have a saved search with two search criteria and you add a third search
criteria between the original two, you'll end up with a second entry for the
second criteria. 

Steps:

1. create a saved search for "subject contains a" and exit the saved search dialog.
2. go to saved search properties, click on the + and add a search for "subject
contains b" and exit the saved search dialog.
3. go to the saved search properites, click the + next to the "a" search to
create an empty entry between "a" and "b". set that search to "subject contains
1" and exit the saved search dialog.
4. go to the saved search properties and see that the searches are now (in
order) "a" "b" "1" "b".  They should be "a" "1" "b". 

See attached screenshots.
Target Milestone: --- → Thunderbird1.1
Attached image setting up the test
Attached image results of test
This doesn't seem to be a problem if you add the new search at the end of the
list of existing criteria.
Flags: blocking-aviary1.1+
Attached patch the fix (obsolete) — Splinter Review
Each search term element (attribute, value, operator, more/less buttons), is
given a unique id when it gets constructed based on the row index. Whenever
items were added or removed, the code tried to re-assign new ids to all of
these elements based on the new index positions. This code was designed to
assume that you could only add or remove search rows at the end of the list and
didn't work very well when we added the ability to add/remove into the middle
of the search term list. 

1) I got rid of all that complicated code, using a global search term counter
to generate unique IDs for each element in the search term. Now, when
constructing a new row, we don't have to re-assign any ids, we can just splice
in the new row into our view (the changes to createSearchRow)

2) as a result of this change, we can no longer assume that the ids of the
more/less buttons on the first row are "more0" and "less0". Instead, we need to
get the listitem at row 0 and get the buttons from that instead of looking them
up by id. (updateRemoveRowButton)

3) onMore and onLess no longer need to know the index to add or remove to so I
got rid of that parameter. Instead, they should figure out which row they are
on and use that as the row index.

4) Methods like resetIdsMore and resetIdsLess are now obsolete because we don't
need to update the ids on the buttons as items are added or removed anymore.

5) I didn't include them in the patch but I changed all of the callers of
onMore to pass in just the event (or null in some cases) instead of passing in
an event and a row number.
Attachment #186263 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 294079
This patch also ends up fixing Bug #294079
Status: NEW → ASSIGNED
I made some changes to get rid of scrollToLastIndex (which is now misnamed)
I now just call:

// the user just added a term, so scroll to it
  gSearchTermList.ensureIndexIsVisible(rowNumber);

directly
Comment on attachment 186263 [details] [diff] [review]
the fix

obsoleting this patch in favor of a better one
Attachment #186263 - Attachment is obsolete: true
Attachment #186263 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch updated fix (obsolete) — Splinter Review
Neil, see comment 4 for more info about the patch.

I also added:

1) an attribute to the search row with the unique identifier associated with
that row. That allows me to dynamically generate the id for the less button
later on. This gets rid of a hack I had in the last patch.

2) Cleaned up some issues with the call to scrollToLastSearchTerm from another
bug.
Attachment #186854 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 186854 [details] [diff] [review]
updated fix

Whoops, I almost overlooked this one. My Mozilla has been inexplicably crashing
more frequently recently, so I forget which bugmail I've read but not acted on
:-(

>+  if (firstListItem)
>+    document.getElementById('less' + firstListItem.getAttribute('uniqueid')).setAttribute('disabled', gTotalSearchTerms == 1);
I almost preferred your previous hack. But I think
firstListItem.lastChild.lastChild.lastChild should get you the element you
want.
I also have a slight preference for .disabled in those cases where it works.

+  var listitem;
+  if (event)
 {
And then you redeclared listitem three lines below. A pity those warnings got
turned off :-(

>+    // get the row index for the listrow 
>+    var listitem = event.target;
>+    while (listitem) 
>+    {
>+      if (listitem.localName == "listitem") 
>+        break;
>+      listitem = listitem.parentNode;
>+    }
>+  }
>+
>+  var rowNumber = listitem ? gSearchTermList.getIndexOfItem(listitem) + 1 : gSearchTermList.getRowCount();
It would be nice (here or to follow up) to factor this out into its own method.
I'm thinking of the initializeTermFromId function here, as instead of walking
through the array it could then use this to get the row index. Eventually I'm
hoping we can ditch ids altogether :-)

>+    // walk through gSearchTerms looking for the term whose searchattribute has our id...
>+    for (var index = 0; index < gSearchTerms.length; index++)
>+    {
>+      if (gSearchTerms[index].obj.searchattribute.id == id)
>+        return initializeTermFromIndex(index);
>+    }
Warning: function does not always return a value.

r=me with the nits fixed.
Attachment #186854 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch updated patchSplinter Review
I updated the patch based on Neil's review comments. Moving forward the r.

Neil, I added a new method called getSearchRowIndexForElement and made onMore,
onLess and initializeTermFromId all go through it to find the list item index
for an arbitrary search term inside the list box.
Attachment #186854 - Attachment is obsolete: true
Attachment #187173 - Flags: superreview?(bienvenu)
Attachment #187173 - Flags: review+
Attachment #187173 - Flags: approval-aviary1.1a2?
Attachment #187173 - Flags: superreview?(bienvenu) → superreview+
Attachment #187173 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.