Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EditorInterface.get_selected_paths() working incorrectly when FileSystemDock is in split mode #94703

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

fstxz
Copy link
Contributor

@fstxz fstxz commented Jul 24, 2024

Fixes #88228

@fstxz fstxz requested a review from a team as a code owner July 24, 2024 16:10
@AThousandShips AThousandShips added bug topic:editor cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jul 24, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Jul 24, 2024
@AThousandShips
Copy link
Member

Will try and test this tomorrow

@fstxz fstxz force-pushed the fix_get_selected_paths branch 2 times, most recently from 93a8967 to ada452a Compare July 24, 2024 17:02
@fstxz fstxz force-pushed the fix_get_selected_paths branch 2 times, most recently from b44d37a to 4d79c90 Compare July 24, 2024 17:10
@fstxz
Copy link
Contributor Author

fstxz commented Jul 25, 2024

I wonder if folders that are selected in the tree should also be included when in split mode? Otherwise you can't select multiple folders that are in different places without changing the display mode to tree only.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed the bug and confirmed that this PR works correctly in both modes (i.e. doesn't regress with the single tree mode selected)

I'd say the expected behavior is that the list of files is returned, but the option of including the folder as well might be considered, though I think we can go with this and see what we might want later as well (we could add a note to the documentation clarifying what exactly is returned, elaborating that it returns just the files selected in split mode)

Approving on that this works and the code looks good, might be a more elegant solution but I'm not familiar with the code here so can't speak to that

@KoBeWi
Copy link
Member

KoBeWi commented Aug 17, 2024

In split mode if you have a folder selected, but none of the files, the returned list is empty. If no file is selected, maybe the selected directory should be returned at least?

@fstxz fstxz force-pushed the fix_get_selected_paths branch from 4d79c90 to b55e97c Compare August 17, 2024 15:32
@fstxz
Copy link
Contributor Author

fstxz commented Aug 17, 2024

@KoBeWi I updated the PR, I hope this is what you meant.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's possible to multi-select in tree in split view, so maybe all selected directories should be returned 🤔
But this way it's fine too.

@fstxz
Copy link
Contributor Author

fstxz commented Aug 17, 2024

Seems like it's possible to multi-select in tree in split view

It's also possible to deselect everything with ctrl, this is why I chose to return ony the current directory.

@akien-mga akien-mga merged commit 955f153 into godotengine:master Aug 19, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@fstxz fstxz deleted the fix_get_selected_paths branch August 19, 2024 12:43
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EditorInterface.get_selected_paths() works incorrectly when FileSystemDock is in split mode
4 participants