Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
102713002:
Support folders in bookmark search (Closed)
|
Created:
7 years ago by d.halman Modified:
6 years, 11 months ago CC:
chromium-reviews, tfarina, browser-components-watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionStrip top level folders from bookmark search
Bookmarks Bar and Other Bookmarks are the only top level folders in the bookmarks tree, they are not user-created and do not support move/delete functionality.
Including folders in bookmark searches also included the top level folders, and would require extra logic to block move/delete actions on these folders.
Since they are top level, they cannot be collapsed in the bookmark tree on the left pane of the bookmark manager (always visible). Showing them in search results as well can be considered redundant, and removing them from search results avoids extra context menu logic as well as edge cases that were ignored when search was originally restricted to urls and not folders.
Also update search results whenever an item in the search results is cut/deleted/restored.
BUG=324231
BUG=329144
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246417
Patch Set 1 #
Total comments: 3
Patch Set 2 : Update search results on delete (test pending) #Patch Set 3 : Add test coverage and improve result refresh #
Total comments: 8
Patch Set 4 : Refresh through backend instead of ui + unit_test coverage #Patch Set 5 : Handle single node restoration and only update once per batch delete #
Total comments: 1
Patch Set 6 : Improve search test comments #Patch Set 7 : Support cut command #Patch Set 8 : Improve removal efficiency #Patch Set 9 : Resolve conflicts with master + .cc handling of clipboard #
Messages
Total messages: 45 (0 generated)
Please review changes in chrome/browser/bookmarks/bookmark_utils.cc
https://codereview.chromium.org/102713002/diff/1/chrome/browser/bookmarks/boo... File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/102713002/diff/1/chrome/browser/bookmarks/boo... chrome/browser/bookmarks/bookmark_utils.cc:87: bool IsTopLevel(const BookmarkNode *n1) { do not use abbreviations. I suggest you change this to: const BookmarkNode* node https://codereview.chromium.org/102713002/diff/1/chrome/browser/bookmarks/boo... chrome/browser/bookmarks/bookmark_utils.cc:235: if (DoesBookmarkContainWords(node, words, languages) && !IsTopLevel(node)) { what if we check is_permanent_node()?
https://codereview.chromium.org/102713002/diff/1/chrome/browser/bookmarks/boo... File chrome/browser/bookmarks/bookmark_utils.cc (right): https://codereview.chromium.org/102713002/diff/1/chrome/browser/bookmarks/boo... chrome/browser/bookmarks/bookmark_utils.cc:87: bool IsTopLevel(const BookmarkNode *n1) { Use BookmarkModel::is_permanent_node, also, update the description and test coverage would be good too.
Switched to using model->is_permanent_node(node), and confirmed expected behavior. Writing BookmarkSearchFolder test now, ran into another edge case: Deleting a parent folder from search results will delete subfolders, but will not remove them if they are showing in the same search result list. Interacting with a subfolder of a deleted parent in search results can crash chromium. I am attempting to remove children of a deleted parent folder from the search results list on delete rather than on refresh.
On 2013/12/05 19:00:59, d.halman wrote: > Switched to using model->is_permanent_node(node), and confirmed expected > behavior. > > Writing BookmarkSearchFolder test now, ran into another edge case: Deleting a > parent folder from search results will delete subfolders, but will not remove > them if they are showing in the same search result list. Interacting with a > subfolder of a deleted parent in search results can crash chromium. I am > attempting to remove children of a deleted parent folder from the search results > list on delete rather than on refresh. "but will not remove them if they are showing in the same search result list" Remove them from where? Bookmarks page? If so, that's a bug in how the page is listening for updates and updating the display.
Sorry for being unclear. The children of the deleted folder are deleted and removed from the bookmarks tree in the left pane, but a reference to the deleted folder's children remains in the search results until the search is refreshed. However, I don't believe the issue is limited to folders: Deleting a url and then undoing the deletion will not add the url back to the search results either. It seems as if the main issue is more related to updating search results after an action has been taken on a member of the results list. I'm investigating whether a simple re-search or refresh of the search results following deletion or undo-deletion will solve both issues.
Fixed issue where children of deleted parents remained in search results until refreshed. Can't find any more edge cases, will finalize test and upload Monday.
Sky + tfarina: Updated test coverage and added result refresh to prevent edge cases. Tests now cover search results of deleted folders containing matching nodes (switched the names of node1 and node3 in the bookmark api test to facilitate greater search test coverage with minimal changes to the test), and delete/undo actions now propagate to children of folder if action is executed from search results with matching children. In short, bookmarks or folders that cannot be moved or deleted (permanent folders and already deleted nodes/folders) no longer show up in search results to allow move/deletion. Note: I could not figure out how to test undo-deletion in the api test. Is this because undo-delete is a function of the bookmark manager and not the bookmark api?
Minimal changes in chrome/browser/bookmarks lgtm. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:830: function updateSearchResults() { you need to get arv or dbeam to review the JS part.
+ dbeam: please review changes in c/b/resources/bookmark_manager/js c/test/data/extensions/api_test/bookmarks
Please add test coverage
-dbeam@/+arv@ because there's too much black magic here (where does |list| even come from, for instance?!)
yeah, I missed that. You should be able add a test in bookmark_utils_unittests.cc showing that the search does not return results containing "root", "Bookmarks Bar", "Mobile", "Other Bookmarks", etc...
Although I see you have coverage and is testing it indirectly in chrome/test/data/extensions/api_test/bookmarks/test.js
dbeam: I'd like to address the black magic issue. I referenced list from other
functions in the file, reading over the surrounding code and checking behavior
in the browser determined the variable "list" refers to the bookmark manager's
right-pane. It displays either search results or the contents of the selected
item in the tree (left-pane). "list.isSearch()" returns true if the active list
is displaying search results, false if it is displaying the children or details
for a selected item in the tree. The point of the updateSearchResults() function
is to refresh the search query in the case that the list is displaying search
results, so that children of deleted folders do not remain in search results
after a deletion. If there is a better way to do this, please let me know and I
will be happy to implement it more cleanly. I can also update other references
to "list" as well if preferred. I'm guessing it's being pulled from the
$('list') object on the page.
sky: The browser test 'chrome/test/data/extensions/api_test/bookmarks/test.js'
for the bookmark manager has been updated to include a search for permanent
nodes, a search for a deleted node, and a search matching both a deleted node
and its child. All 3 searches return 0 results, after verifying earlier in the
test that the same search queries for the deleted node and deleted node/child
pair returned non-zero results. Is this insufficient?
tfarina: Would you recommend moving the previously mentioned tests to a unit
test, implementing a unit test in parallel to the changes to the browser test,
or is the added coverage to the browser test sufficient?
+arv: please advise on comments regarding c/b/resources/bookmark_manager/js/main.js per dbeam's request.
I am not a good reviewer for the JS side. As to test coverage, add local test coverage in bookmark_utils.
Halman, yes, as you are changing the behavior of GetBookmarksContainingText(), it would be good to add a C++ unit test for it, to make sure we don't regress in future.
Why is this one CL instead of two? I feel like change to not include the top level folders in the search results and updating the BMM search results on changes are orthogonal. On the BMM side the search results are already updated when a bookmark node is removed (from inside or outside the bmm). The search result is not updated when a new item is added that matches the search result (non search result lists are updated). However, tearing down the current result and rebuilding it is not the right approach. A better way is to just updated the dataModel. Take a look at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... for how this is done for the much simpler case of non search results. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:834: $('term').focus(); This is not very nice. Why are we changing focus here? My concern is that the focus may be somewhere else and the user deletes a bookmark and now the focus is moved. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:861: updateSearchResults(); I don't understand this. Currently, if I'm deleting an item from the search results it is removed from the result. Why do we need to refresh the search results? Is this when the use deletes a bookmark from the bookmarks toolbar? (no, that also works) https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:892: updateSearchResults(); This one I think is needed (undo delete does not update the search results). However, updateSearchResults does not seem like the optimal way to handle this. It would be better if we could check if any of the new bookmarks would be shown in the search result and just update the data model. That way focus and selection is not reset.
Thank you for the input and review, arv. I'll attempt to refactor updateSearchResults to manipulate the data model directly rather than executing ui commands. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:834: $('term').focus(); This was a somewhat hacky way to force search results to update. I'll look into modifying the data model without stealing focus or rebuilding the search. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:861: updateSearchResults(); I understand your other comments referring to updating the model. I agree that is the best approach to this problem and I will look into doing so. To explain this particular line: yes, deleting a bookmark will update the data model and remove it from search results. However, deleting a folder of bookmarks from search results will remove the folder without removing any children of the folder that may also be showing in the results. This behavior resulted in deleted bookmarks remaining in the results list, allowing users to interact with them and crash chromium. I agree the root-level folder stripping is orthogonal to updating search results, but both of the issues exhibit the same crash behavior and stem from the last change I made to include folders in search results. I can make a separate CL if that is the proper approach. https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:892: updateSearchResults(); Would there be an efficient way to check restored nodes against the current search parameters in the case where a folder is restored while a search is active? Or would running a single search once the action has been completed be preferable?
https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... File chrome/browser/resources/bookmark_manager/js/main.js (right): https://codereview.chromium.org/102713002/diff/40001/chrome/browser/resources... chrome/browser/resources/bookmark_manager/js/main.js:892: updateSearchResults(); On 2013/12/13 21:04:07, d.halman wrote: > Would there be an efficient way to check restored nodes against the current > search parameters in the case where a folder is restored while a search is > active? Not at the moment AFIAK. > Or would running a single search once the action has been completed be > preferable? I think that would be the best way to handle this at this point.
Looking through bookmark_list.js that you linked previously, I see a reload()
function that handles list updating without refocusing or re-executing UI
actions like navigation. Testing, list.reload() works as a drop-in replacement
for updateSearchResults() in this patch. Would this be acceptable?
In its current implementation, the list is only reloaded if search is active. It
is reloaded once after all nodes and children have been deleted (since we know
how many nodes are being deleted and can fire the reload in the last delete's
callback), and it is reloaded once for each non-empty folder that is un-deleted
(I was not able to find an easy way to recognize when the last callback is being
executed in un-delete). This is the same behavior as the last patch, but it does
not use $('term').focus() or navigateTo().
On 2013/12/13 23:11:15, d.halman wrote:
> Looking through bookmark_list.js that you linked previously, I see a reload()
> function that handles list updating without refocusing or re-executing UI
> actions like navigation. Testing, list.reload() works as a drop-in replacement
> for updateSearchResults() in this patch. Would this be acceptable?
>
> In its current implementation, the list is only reloaded if search is active.
It
> is reloaded once after all nodes and children have been deleted (since we know
> how many nodes are being deleted and can fire the reload in the last delete's
> callback), and it is reloaded once for each non-empty folder that is
un-deleted
> (I was not able to find an easy way to recognize when the last callback is
being
> executed in un-delete). This is the same behavior as the last patch, but it
does
> not use $('term').focus() or navigateTo().
This still breaks selection, doesn't it?
On 2013/12/16 17:12:30, arv wrote: > This still breaks selection, doesn't it? On delete, the selected items are removed from results and could not remain selected, so I'm not sure if that's an issue. On undo-delete, the action is performed on items that don't currently exist. Since the undo action is global, it can be performed while another item in the search results is selected. Therefore, yes, performing an undo action will de-select anything previously selected in search results. However, the undo action cannot be performed on anything currently selected, so I'm unclear on what the expected behavior should be. Which is preferable: this current behavior, storing a selection state and restoring it after the list is reloaded, or selecting the item(s) restored by the undo action? I am also open to other suggestions for expected behavior. It should also be noted that these behaviors are only exhibited if search results are showing in the list, which is only if the "Search" option is selected in the tree, so tree selection is unaffected.
On 2013/12/16 21:15:02, d.halman wrote: > On 2013/12/16 17:12:30, arv wrote: > > This still breaks selection, doesn't it? > > On delete, the selected items are removed from results and could not remain > selected, so I'm not sure if that's an issue. > > On undo-delete, the action is performed on items that don't currently exist. > Since the undo action is global, it can be performed while another item in the > search results is selected. Therefore, yes, performing an undo action will > de-select anything previously selected in search results. However, the undo > action cannot be performed on anything currently selected, so I'm unclear on > what the expected behavior should be. > > Which is preferable: this current behavior, storing a selection state and > restoring it after the list is reloaded, or selecting the item(s) restored by > the undo action? I am also open to other suggestions for expected behavior. I was hoping we could just update the data model? > It should also be noted that these behaviors are only exhibited if search > results are showing in the list, which is only if the "Search" option is > selected in the tree, so tree selection is unaffected. Yeah, maybe it is not so bad to reset the selection since these things are pretty rare.
On 2013/12/16 23:33:21, arv wrote: > I was hoping we could just update the data model? Playing around with it for a bit, I was able to modify the data model for delete and undo-delete but I was unable to find an efficient way to check the conditions: for deletion, it requires recursively traversing through all descendants and removing each individually (which is possible, but I don't know if there's an advantage over reloading the list), and for undo-deletion it requires checking each restored node against the search query (which I'm not sure how to do other than running a search again). > Yeah, maybe it is not so bad to reset the selection since these things are > pretty rare. The only time I've been able to verify selection resetting is: 1: run a bookmark search that returns a folder, one of its children, and another node that is not one of its children in the same results list. 2: right click the parent folder and delete (leaves the non-child in the results) 3: click on any remaining result to select 4: right click on white space below results and click undo 5: observe deselection of selected item I see this being unexpected if the user thought "undo" referred to some action being performed on the selected item, but I'm not sure that's a reasonable case. Please let me know if you'd like me to further pursue updating the data model or if you have any suggestions on how to improve the conditions. It may be reasonable to modify handleRemoved() to prevent a second recursion traversal, but I'd probably need help in finding a better way to check against the search query in handleCreated(). Do you suggest this over the current list.reload() implementation? The only difference I have noted is the dataModel.splice() implementation will leave the selected item from Step3 above selected after the undo has been performed. Lastly, the best idea I can come up with to prevent repeated calls to updateSearchResults() is to count each tree pushed to the lastRemoved list and perform updateSearchResults() once when the restored tree count is equal to the previous removed tree count. However, the list updating in real time as each bookmark is restored may actually be a preferable UX to clicking undo and then waiting several seconds before anything happens.
On Mon, Dec 16, 2013 at 10:05 PM, <d.halman@gmail.com> wrote: > On 2013/12/16 23:33:21, arv wrote: >> >> I was hoping we could just update the data model? > > > Playing around with it for a bit, I was able to modify the data model for > delete > and undo-delete but I was unable to find an efficient way to check the > conditions: for deletion, it requires recursively traversing through all > descendants and removing each individually (which is possible, but I don't > know > if there's an advantage over reloading the list), and for undo-deletion it > requires checking each restored node against the search query (which I'm not > sure how to do other than running a search again). Could you do the search again and diff the result? Was that the issue where we do not know when the undo is done? >> Yeah, maybe it is not so bad to reset the selection since these things are >> pretty rare. > > > The only time I've been able to verify selection resetting is: > 1: run a bookmark search that returns a folder, one of its children, and > another node that is not one of its children in the same results list. > 2: right click the parent folder and delete (leaves the non-child in the > results) > 3: click on any remaining result to select > 4: right click on white space below results and click undo > 5: observe deselection of selected item > > I see this being unexpected if the user thought "undo" referred to some > action > being performed on the selected item, but I'm not sure that's a reasonable > case. > > > Please let me know if you'd like me to further pursue updating the data > model or > if you have any suggestions on how to improve the conditions. It may be > reasonable to modify handleRemoved() to prevent a second recursion > traversal, > but I'd probably need help in finding a better way to check against the > search > query in handleCreated(). Do you suggest this over the current list.reload() > implementation? The only difference I have noted is the dataModel.splice() > implementation will leave the selected item from Step3 above selected after > the > undo has been performed. > > Lastly, the best idea I can come up with to prevent repeated calls to > updateSearchResults() is to count each tree pushed to the lastRemoved list > and > perform updateSearchResults() once when the restored tree count is equal to > the > previous removed tree count. However, the list updating in real time as each > bookmark is restored may actually be a preferable UX to clicking undo and > then > waiting several seconds before anything happens. > > https://codereview.chromium.org/102713002/ I think we should just go ahead and reload the search view. If people find that too confusing we can revisit this. -- erik To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I think we should just go ahead and reload the search view. If people > find that too confusing we can revisit this. > > -- > erik Alright. I was going to make one more change that fixed the case where single nodes are un-deleted and don't pop back up in the results because they don't have children. Are there any other suggested changes necessary for an LG?
LGTM
sky: test coverage sufficient?
Minor nits, LGTM otherwise https://codereview.chromium.org/102713002/diff/80001/chrome/browser/bookmarks... File chrome/browser/bookmarks/bookmark_utils_unittest.cc (right): https://codereview.chromium.org/102713002/diff/80001/chrome/browser/bookmarks... chrome/browser/bookmarks/bookmark_utils_unittest.cc:68: // Ensure Bookmark Bar and Other Bookmarks do not return in search results 'do not return' -> 'are not returned'. And end sentence with a '.'.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/d.halman@gmail.com/102713002/90001
Failed to apply patch for chrome/browser/bookmarks/bookmark_utils.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/bookmarks/bookmark_utils.cc
Hunk #1 FAILED at 230.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_utils.cc.rej
Patch: chrome/browser/bookmarks/bookmark_utils.cc
Index: chrome/browser/bookmarks/bookmark_utils.cc
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc
b/chrome/browser/bookmarks/bookmark_utils.cc
index
5aa4f07ab183d1d7b44449076d313b1d8945271b..9a6c7634c64e8a26ef84d10c896366eb7d1a08bc
100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -230,7 +230,8 @@ void GetBookmarksContainingText(BookmarkModel* model,
ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
while (iterator.has_next()) {
const BookmarkNode* node = iterator.Next();
- if (DoesBookmarkContainWords(node, words, languages)) {
+ if (DoesBookmarkContainWords(node, words, languages) &&
+ !model->is_permanent_node(node)) {
nodes->push_back(node);
if (nodes->size() == max_count)
return;
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/d.halman@gmail.com/102713002/110001
Failed to apply patch for chrome/browser/bookmarks/bookmark_utils.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/bookmarks/bookmark_utils.cc
Hunk #1 FAILED at 230.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_utils.cc.rej
Patch: chrome/browser/bookmarks/bookmark_utils.cc
Index: chrome/browser/bookmarks/bookmark_utils.cc
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc
b/chrome/browser/bookmarks/bookmark_utils.cc
index
5aa4f07ab183d1d7b44449076d313b1d8945271b..9a6c7634c64e8a26ef84d10c896366eb7d1a08bc
100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -230,7 +230,8 @@ void GetBookmarksContainingText(BookmarkModel* model,
ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
while (iterator.has_next()) {
const BookmarkNode* node = iterator.Next();
- if (DoesBookmarkContainWords(node, words, languages)) {
+ if (DoesBookmarkContainWords(node, words, languages) &&
+ !model->is_permanent_node(node)) {
nodes->push_back(node);
if (nodes->size() == max_count)
return;
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/d.halman@gmail.com/102713002/130001
Failed to apply patch for chrome/browser/bookmarks/bookmark_utils.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/bookmarks/bookmark_utils.cc
Hunk #1 FAILED at 230.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_utils.cc.rej
Patch: chrome/browser/bookmarks/bookmark_utils.cc
Index: chrome/browser/bookmarks/bookmark_utils.cc
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc
b/chrome/browser/bookmarks/bookmark_utils.cc
index
5aa4f07ab183d1d7b44449076d313b1d8945271b..6e1a0ac16d99b74833dccae95918e0322506d9f4
100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -230,7 +230,8 @@ void GetBookmarksContainingText(BookmarkModel* model,
ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
while (iterator.has_next()) {
const BookmarkNode* node = iterator.Next();
- if (DoesBookmarkContainWords(node, words, languages)) {
+ if (!model->is_permanent_node(node)) &&
+ DoesBookmarkContainWords(node, words, languages) {
nodes->push_back(node);
if (nodes->size() == max_count)
return;
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/d.halman@gmail.com/102713002/140001
Failed to apply patch for chrome/browser/bookmarks/bookmark_utils.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/bookmarks/bookmark_utils.cc
Hunk #1 FAILED at 230.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/bookmarks/bookmark_utils.cc.rej
Patch: chrome/browser/bookmarks/bookmark_utils.cc
Index: chrome/browser/bookmarks/bookmark_utils.cc
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc
b/chrome/browser/bookmarks/bookmark_utils.cc
index
5aa4f07ab183d1d7b44449076d313b1d8945271b..f8425d2b6e72c9c8f65968df8d89ff1e9432c0d9
100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -230,7 +230,8 @@ void GetBookmarksContainingText(BookmarkModel* model,
ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
while (iterator.has_next()) {
const BookmarkNode* node = iterator.Next();
- if (DoesBookmarkContainWords(node, words, languages)) {
+ if (!model->is_permanent_node(node) &&
+ DoesBookmarkContainWords(node, words, languages)) {
nodes->push_back(node);
if (nodes->size() == max_count)
return;
+tom for reviewer working on same area of code. Added support for the cut command (wasn't updating similar to delete) and also fixed a bug in batch-cut/copy where duplicates were causing issues.
On 2013/12/21 00:37:44, d.halman wrote: > +tom for reviewer working on same area of code. > > Added support for the cut command (wasn't updating similar to delete) and also > fixed a bug in batch-cut/copy where duplicates were causing issues. This change will not interfere in any way with the undo work that is in progress. One comment; there is benefit in handling this in bookmark_utils::CopyToClipboard instead in the bmm javascript. The likelihood for new (future) code that would allow cut/copy of bookmarks at different levels is probably low, but if it does happen this exact problem would manifest if the code isn't handled at bookmark_utils::CopyToClipboard.
Will look into implementing before the writeToClipboard command in copyToClipboard.
Tom: I handled copyToClipboard from the .cc side with a filter similar to the one I wrote (and since improved) for the .js side. It's reasonably fast and handles everything but delete/undo (which are handled purely in main.js) so I left the .js filter in for that case. Search result nested tree batch deletion, cutting, copying, and pasting no longer crashes or produces duplicates. Restoring batch deletions no longer produces duplicates or fails to restore a copied subtree, and moving was never observed causing an issue.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/d.halman@gmail.com/102713002/270001
Message was sent while issue was closed.
Change committed as 246417 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
