Allow removing files in the file search #82629

Stronkkey · 2023-10-01T17:05:34Z

editor/find_in_files.cpp

Calinou

Tested locally (rebased on top of bfd78bb), it works as expected.

Replace in Files also takes deleted entries into account:

Result:

The icon design should be changed though, as it doesn't fit well with the rest of the engine's visual design currently (see review comment).

Stronkkey · 2023-10-05T05:25:49Z

Is the new version good?

Calinou

Looks great now!

editor/find_in_files.cpp

akien-mga · 2023-10-09T13:05:14Z

It may look nicer if the button was added on hover only, like in VS Code in KoBeWi's proposal. But if that requires making the implementation significantly more complex, then it's probably not worth it.

KoBeWi · 2023-10-09T13:30:22Z

Another thing is that in VS Code deleting a result will update the number:

Code_sNIFvD4Ng4.mp4

editor/find_in_files.cpp

KoBeWi · 2023-10-09T20:40:52Z

Deleting all entries under a file should also delete the file:

godot.windows.editor.dev.x86_64_B39QGrj5Tf.mp4

Also deleting a file with many results seems to cause a short freeze and error spam 🤔

Stronkkey · 2023-10-10T11:22:10Z

@KoBeWi Can't replicate this issue and also it's kind of expected when you are hiding files with a lot of matches (10k+).

Deleting all entries under a file should also delete the file:

I don't think this is possible with the current implementation of TreeItem and Tree.

KoBeWi · 2023-10-10T11:35:19Z

Can't replicate this issue and also it's kind of expected when you are hiding files with a lot of matches (10k+).

godot.windows.editor.dev.x86_64_9wnf582zyo.mp4

I deleted 39 out of 600 results. It's far from thousands.
The error spam might be the cuprit.

I don't think this is possible with the current implementation of TreeItem and Tree.

Why? When deleting item, check if it's the only item. If not, delete item. If true, delete parent instead.

Stronkkey · 2023-10-10T15:09:56Z

I deleted 39 out of 600 results. It's far from thousands.

I think this has to do with some other piece of code that is causing all these errors because when I test it out, it works fine without any errors.

KoBeWi · 2023-10-10T15:15:31Z

I'm testing on newest master, so maybe try rebasing if you are not up-to-date.

Stronkkey · 2023-10-10T18:02:59Z

@KoBeWi Can't seem to be able to replicate it. Also can someone rerun the checks?

KoBeWi · 2023-10-12T09:59:41Z

Your newest changes fixed the errors and lag, but deleting file does not update matches:

godot.windows.editor.dev.x86_64_Zz0R1HGs2y.mp4

Stronkkey · 2023-10-12T16:39:39Z

@KoBeWi Should be fixed in the newest but I doubt this will get merged before 4.2

AThousandShips · 2023-10-12T16:40:42Z

It's an enhancement and we're in feature freeze so it won't be merged until 4.3, no rush to fix anything but nothing stopping working on it either :)

editor/find_in_files.cpp

KoBeWi · 2023-10-12T17:25:23Z

editor/find_in_files.cpp

+	if (_result_items.find(p_item)) {
+		_result_items.erase(p_item);


You can avoid double lookup if you do

Suggested change

if (_result_items.find(p_item)) {

_result_items.erase(p_item);

HashMap<TreeItem *, Result>::Iterator E = _result_items.find(p_item);

if (E) {

_result_items.remove(E);

EDIT:
Also this piece of code is repeated so many times that you could extract it to a method, e.g. _delete_result_item(item).

KoBeWi

Left one minor comment, but otherwise it's fine.

AThousandShips · 2023-10-21T10:27:18Z

editor/find_in_files.cpp

+		_file_items.erase(file_path);
+	}
+
+	get_tree()->queue_delete(p_item);


I'd move this below as well just to ensure that we don't operate on something after we've queued the deletion, just for correctness and safety

AThousandShips

LGTM otherwise

akien-mga · 2024-01-04T14:51:40Z

@Stronkkey Are you available to address the last review comments?

Stronkkey requested a review from a team as a code owner October 1, 2023 17:05

Stronkkey mentioned this pull request Oct 1, 2023

Add the ability to remove files when using file search #82621

Closed

Chaosus added enhancement topic:editor usability labels Oct 2, 2023

Chaosus added this to the 4.2 milestone Oct 2, 2023

Stronkkey changed the title ~~Adds the ability to remove files when using file search~~ Add the ability to remove files when using file search Oct 2, 2023

Stronkkey force-pushed the remove_entries branch 2 times, most recently from a8e9a72 to 531676b Compare October 2, 2023 09:21

Stronkkey changed the title ~~Add the ability to remove files when using file search~~ Allowing removing files in the file search Oct 3, 2023

Calinou reviewed Oct 4, 2023

View reviewed changes

editor/find_in_files.cpp Outdated Show resolved Hide resolved

Calinou requested changes Oct 4, 2023

View reviewed changes

Stronkkey force-pushed the remove_entries branch from 531676b to b4c4ad8 Compare October 5, 2023 05:24

Stronkkey changed the title ~~Allowing removing files in the file search~~ Allow removing files in the file search Oct 5, 2023

Stronkkey requested a review from Calinou October 6, 2023 18:29

Calinou approved these changes Oct 7, 2023

View reviewed changes

AThousandShips reviewed Oct 7, 2023

View reviewed changes

editor/find_in_files.cpp Outdated Show resolved Hide resolved

Stronkkey force-pushed the remove_entries branch from 3b5162d to 1e52f24 Compare October 7, 2023 13:30

Stronkkey requested a review from AThousandShips October 7, 2023 13:32

akien-mga reviewed Oct 9, 2023

View reviewed changes

editor/find_in_files.cpp Outdated Show resolved Hide resolved

akien-mga reviewed Oct 9, 2023

View reviewed changes

editor/find_in_files.cpp Show resolved Hide resolved

KoBeWi reviewed Oct 9, 2023

View reviewed changes

editor/find_in_files.cpp Outdated Show resolved Hide resolved

Stronkkey force-pushed the remove_entries branch 4 times, most recently from 4cef9a8 to 91d15e3 Compare October 9, 2023 19:50

KoBeWi mentioned this pull request Oct 9, 2023

Allow clicking buttons of non-selectable TreeItems #83065

Merged

akien-mga modified the milestones: 4.2, 4.x Oct 10, 2023

Stronkkey force-pushed the remove_entries branch from 91d15e3 to 1f76b78 Compare October 10, 2023 11:24

Stronkkey force-pushed the remove_entries branch from 1f76b78 to b1a031a Compare October 10, 2023 15:16

Stronkkey force-pushed the remove_entries branch from b1a031a to 190e0e6 Compare October 12, 2023 16:38

AThousandShips reviewed Oct 12, 2023

View reviewed changes

editor/find_in_files.cpp Outdated Show resolved Hide resolved

Adds the ability to remove files when using file search

1da3ddb

Stronkkey force-pushed the remove_entries branch from 190e0e6 to 1da3ddb Compare October 12, 2023 16:45

KoBeWi reviewed Oct 12, 2023

View reviewed changes

KoBeWi approved these changes Oct 12, 2023

View reviewed changes

akien-mga modified the milestones: 4.x, 4.3 Oct 13, 2023

AThousandShips reviewed Oct 21, 2023

View reviewed changes

AThousandShips approved these changes Oct 21, 2023

View reviewed changes

Stronkkey closed this Jan 9, 2024

AThousandShips added the archived label Jan 9, 2024

AThousandShips removed this from the 4.3 milestone Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow removing files in the file search #82629

Allow removing files in the file search #82629

Stronkkey commented Oct 1, 2023 •

edited

Loading

Calinou left a comment •

edited

Loading

Stronkkey commented Oct 5, 2023

Calinou left a comment

akien-mga commented Oct 9, 2023

KoBeWi commented Oct 9, 2023

KoBeWi commented Oct 9, 2023

Stronkkey commented Oct 10, 2023 •

edited

Loading

KoBeWi commented Oct 10, 2023

Stronkkey commented Oct 10, 2023

KoBeWi commented Oct 10, 2023

Stronkkey commented Oct 10, 2023

KoBeWi commented Oct 12, 2023

Stronkkey commented Oct 12, 2023

AThousandShips commented Oct 12, 2023

KoBeWi Oct 12, 2023 •

edited

Loading

KoBeWi left a comment

AThousandShips Oct 21, 2023

AThousandShips left a comment

akien-mga commented Jan 4, 2024

		if (_result_items.find(p_item)) {
		_result_items.erase(p_item);

-	if (_result_items.find(p_item)) {
-		_result_items.erase(p_item);
+	HashMap<TreeItem *, Result>::Iterator E = _result_items.find(p_item);
+	if (E) {
+		_result_items.remove(E);

Allow removing files in the file search #82629

Allow removing files in the file search #82629

Conversation

Stronkkey commented Oct 1, 2023 • edited Loading

Calinou left a comment • edited Loading

Choose a reason for hiding this comment

Stronkkey commented Oct 5, 2023

Calinou left a comment

Choose a reason for hiding this comment

akien-mga commented Oct 9, 2023

KoBeWi commented Oct 9, 2023

KoBeWi commented Oct 9, 2023

Stronkkey commented Oct 10, 2023 • edited Loading

KoBeWi commented Oct 10, 2023

Stronkkey commented Oct 10, 2023

KoBeWi commented Oct 10, 2023

Stronkkey commented Oct 10, 2023

KoBeWi commented Oct 12, 2023

Stronkkey commented Oct 12, 2023

AThousandShips commented Oct 12, 2023

KoBeWi Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

KoBeWi left a comment

Choose a reason for hiding this comment

AThousandShips Oct 21, 2023

Choose a reason for hiding this comment

AThousandShips left a comment

Choose a reason for hiding this comment

akien-mga commented Jan 4, 2024

Stronkkey commented Oct 1, 2023 •

edited

Loading

Calinou left a comment •

edited

Loading

Stronkkey commented Oct 10, 2023 •

edited

Loading

KoBeWi Oct 12, 2023 •

edited

Loading