Skip to content

Conversation

jusso-dev
Copy link
Contributor

Adds enhancement for issue #1340

#1340

Added warning to warn users before changing file extension. Mirrors the wording of the classic Windows File Explorer.

I'm also happy to:

  • Move warning message to common strings file
  • Create separate private method that contains popup dialog logic
  • Perform any other changes to conform with project standards

Great project! thanks a lot for the review :)

@yaira2 yaira2 requested a review from tsvietOK February 1, 2021 20:54
@yaira2 yaira2 added the changes requested Changes are needed for this pull request label Feb 2, 2021
…e and used pre-existing create dialog helper
@jusso-dev jusso-dev requested a review from yaira2 February 2, 2021 10:47
@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 2, 2021

@jusso-dev Seems like shortcut renaming is now broken.

@jusso-dev
Copy link
Contributor Author

@jusso-dev Seems like shortcut renaming is now broken.

I'm investigating a clean approach to fix this.

@jusso-dev jusso-dev requested a review from tsvietOK February 2, 2021 11:44
@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 2, 2021

@jusso-dev Now renaming doesn't working at all.

@yaira2 yaira2 added needs - code review and removed changes requested Changes are needed for this pull request labels Feb 2, 2021
@jusso-dev
Copy link
Contributor Author

@tsvietOK - Pushed another fix, things I manually tested as working:

Renaming files - ✅
Renaming Shortcuts - ✅
Renaming folders - ✅
Selecting Yes on rename of file, folders and shortcuts - ✅
Select No on rename of file, folders and shortcuts - ✅

@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 3, 2021

@jusso-dev Now dialog is displayed when changing file name without extension change.

@tsvietOK tsvietOK added the changes requested Changes are needed for this pull request label Feb 3, 2021
@jusso-dev
Copy link
Contributor Author

@tsvietOK When you are free I would appreciate you reviewing this again and merging if possible

@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 7, 2021

@jusso-dev Now dialog is not displayed when changing file extension. By the way, resolve conflicts, please.

@jusso-dev
Copy link
Contributor Author

@tsvietOK - Hate to be that person but it's working for me, every test I run locally works, so not sure what to say. If it's still not working for you I'll close this PR.

@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 8, 2021

@jusso-dev I'll check again a bit later.

@d2dyno1
Copy link
Member

d2dyno1 commented Feb 8, 2021

image
😳

@jusso-dev
Copy link
Contributor Author

@d2dyno1 I don't know how that happened, all these commits, merges, merge conflicts have stuffed this pull request, my change is small less than 20 line code change, plus some other unnecessary, unrelated changes that were introduced into this specific branch.

@tsvietOK tsvietOK force-pushed the jusso-dev/add-alertdialog-file-extensions branch from 57e344e to 28d8075 Compare February 8, 2021 10:16
@tsvietOK
Copy link
Contributor

tsvietOK commented Feb 8, 2021

Branch fixed.

@tsvietOK tsvietOK added ready to merge Pull requests that are approved and ready to merge and removed changes requested Changes are needed for this pull request labels Feb 8, 2021
@yaira2 yaira2 changed the title Added Popup dialog to warn users about changing file extensions Added a dialog to warn users when changing file extensions Feb 9, 2021
@yaira2 yaira2 merged commit d3cc857 into files-community:main Feb 9, 2021
@yaira2
Copy link
Member

yaira2 commented Feb 9, 2021

@jusso-dev Thank you, great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants