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.

Sign up for GitHub

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

Jump to bottom

Fix several issues with renaming in FileSystem dock #82075

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Fix several issues with renaming in FileSystem dock #82075

merged 1 commit into from
Oct 9, 2023

Conversation

VedatGunel
Copy link
Contributor

@VedatGunel VedatGunel commented Sep 21, 2023 •
edited
Loading

Fixes #82074
Fixes #82543
I tested all the cases I could think of, with Single Window Mode on/off & Split Mode on/off, and didn't spot any regressions.

@VedatGunel VedatGunel requested a review from a team as a code owner September 21, 2023 19:04
@akien-mga akien-mga added this to the 4.2 milestone Sep 21, 2023
@VedatGunel VedatGunel marked this pull request as draft September 21, 2023 20:36
@VedatGunel VedatGunel force-pushed the fix-filesystem-rename-crash branch 2 times, most recently from 2e7dc07 to 9c3ec93 Compare September 21, 2023 21:32
@VedatGunel VedatGunel marked this pull request as ready for review September 22, 2023 05:57
@VedatGunel VedatGunel marked this pull request as draft September 22, 2023 06:05
@VedatGunel VedatGunel force-pushed the fix-filesystem-rename-crash branch 2 times, most recently from 840691e to 7435659 Compare September 24, 2023 17:00
@VedatGunel VedatGunel marked this pull request as ready for review September 24, 2023 17:29
@VedatGunel VedatGunel mentioned this pull request Oct 7, 2023
Closed
@VedatGunel
Copy link
Contributor Author

VedatGunel commented Oct 7, 2023 •
edited
Loading

Rebased to replace the focus check that was merged with #81007, similar to the others.
Checking if the tree has focus doesn't work reliably when clicking outside the FileSystem dock, but checking the edited item always works.

@VedatGunel VedatGunel changed the title Fix crash when renaming a file/folder by clicking outside Fix several issues with renaming in FileSystem dock Oct 7, 2023
@Sauermann
Copy link
Contributor

It is good to see, that we came to the same conclusion of how to fix #82543.

@VedatGunel
Copy link
Contributor Author

Included an experimental fix for #19592.
I couldn't reproduce the issue on Windows 10 or Windows 11, so I included the macOS preprocessor check.
Can any macOS users test it?

@akien-mga
Copy link
Member

Included an experimental fix for #19592. I couldn't reproduce the issue on Windows 10 or Windows 11, so I included the macOS preprocessor check. Can any macOS users test it?

CC @bruvzg

bruvzg
bruvzg reviewed Oct 7, 2023
View reviewed changes
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Included an experimental fix for #19592.
I couldn't reproduce the issue on Windows 10 or Windows 11, so I included the macOS preprocessor check.
Can any macOS users test it?

It is fixing it on macOS, on a case-insensitive file system. But it will overwrite file without any warnings on case-sensitive FS!

@bruvzg
Copy link
Member

bruvzg commented Oct 7, 2023

It is fixing it on macOS, on a case-insensitive file system. But it will overwrite file without any warnings on case-sensitive FS!

Probably require something more complex (work on both case-insensitive and case-sensitive fs on macOS, have not tested it on Windows).

#82957

@VedatGunel VedatGunel force-pushed the fix-filesystem-rename-crash branch 2 times, most recently from 4fc4baf to 7038d90 Compare October 7, 2023 11:43
@VedatGunel
Copy link
Contributor Author

Thanks for the review and quick PR!
I reverted the macOS change.

@VedatGunel
Copy link
Contributor Author

Included a fix for #82543 (comment), which is a recent regression from #82045.
I'm not sure if it's the best solution though, I'm open to suggestions.

akien-mga
akien-mga approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Awesome work! I can confirm this fixes both issues for me.

I didn't review the code changes in depth (not very familiar with this code), review welcome from @KoBeWi @groud (and @Sauermann since you looked into this too).

@akien-mga
Copy link
Member

BTW now that you're familiar with that code @VedatGunel, you might be interested to look into #82774 (but if you do it's probably best left for a follow-up PR, as this one is fixing a regression while #82774 seems to be a pre-existing issue).

KoBeWi
KoBeWi reviewed Oct 9, 2023
View reviewed changes
editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Oct 9, 2023
View reviewed changes
editor/filesystem_dock.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 98287fe into godotengine:master Oct 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@VedatGunel VedatGunel deleted the fix-filesystem-rename-crash branch October 9, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruvzg bruvzg bruvzg left review comments

@akien-mga akien-mga akien-mga approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug crash topic:editor
Projects
None yet
Milestone
4.2
5 participants
@VedatGunel @Sauermann @akien-mga @bruvzg @KoBeWi