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)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird1.1
People
(Reporter: asa, Assigned: mscott)
References
Details
Attachments
(3 files, 2 obsolete files)
8.54 KB,
image/png
|
Details | |
8.71 KB,
image/png
|
Details | |
10.38 KB,
patch
|
mscott
:
review+
Bienvenu
:
superreview+
asa
:
approval-aviary1.1a2+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Target Milestone: --- → Thunderbird1.1
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Reporter | ||
Comment 3•19 years ago
|
||
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+
Assignee | ||
Comment 4•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #186263 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 6•19 years ago
|
||
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
Assignee | ||
Comment 7•19 years ago
|
||
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)
Assignee | ||
Comment 8•19 years ago
|
||
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 9•19 years ago
|
||
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+
Assignee | ||
Comment 10•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #186854 -
Attachment is obsolete: true
Attachment #187173 -
Flags: superreview?(bienvenu)
Attachment #187173 -
Flags: review+
Attachment #187173 -
Flags: approval-aviary1.1a2?
Updated•19 years ago
|
Attachment #187173 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•19 years ago
|
Attachment #187173 -
Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Assignee | ||
Updated•19 years ago
|
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.
Description
•