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

wxGUI/animation: add export output animation file validation, before export file #839

Merged
merged 1 commit into from
Aug 12, 2020
Merged

wxGUI/animation: add export output animation file validation, before export file #839

merged 1 commit into from
Aug 12, 2020

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Jul 27, 2020

To Reproduce

  1. Create an animation
  2. Go to export
  3. Choose export to animated GIF
  4. Set output GIF file /tmp/test (but without postfix '.gif')
  5. Hit Export button

Error message:

g_gui_anim_export_error

Expected behavior:

Validate exported output animation file name/path.

@neteler neteler requested a review from veroandreo July 27, 2020 11:23
@neteler neteler added this to the 7.8.4 milestone Jul 27, 2020
@neteler neteler added the GUI wxGUI related label Jul 27, 2020
@tmszi tmszi force-pushed the fix_g_gui_anim_check_exp_file_postfix branch from 924db5c to adc82fe Compare July 27, 2020 18:56
@neteler
Copy link
Member

neteler commented Aug 11, 2020

@veroandreo does this fix address the issue?

petrasovaa
petrasovaa reviewed Aug 12, 2020
View reviewed changes
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

While the validation would work, wouldn't it be better to just check for the extension and if the extension is not there, just add it to the file path? That would be more inline with how other programs deal with that.

@tmszi tmszi force-pushed the fix_g_gui_anim_check_exp_file_postfix branch from adc82fe to 0e6b2d4 Compare August 12, 2020 04:53
@tmszi
Copy link
Member Author

tmszi commented Aug 12, 2020 •
edited
Loading

While the validation would work, wouldn't it be better to just check for the extension and if the extension is not there, just add it to the file path? That would be more inline with how other programs deal with that.

Yes, this is true, but in the default behavior, an error message dialog is showed when you press the export button and the export dialog is closed. It's a little annoying reopening the export dialog again.

Yes, a good idea. I've already fixed it.

petrasovaa
petrasovaa requested changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

Thanks, this works great. I can see it's based on relesebranch_7_8, could you rebase it to master please?

gui/wxpython/animation/dialogs.py Outdated
GError(parent=self, message=_("Export file is missing."))
return False
else:
if file_postfix not in file_path:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably use endswith(), I can imagine users naming their files with e.g. 'gif' in the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably use endswith(), I can imagine users naming their files with e.g. 'gif' in the name.

Yes of course.

@tmszi tmszi force-pushed the fix_g_gui_anim_check_exp_file_postfix branch 3 times, most recently from cd663f8 to 42c04f5 Compare August 12, 2020 14:17
@tmszi
Copy link
Member Author

tmszi commented Aug 12, 2020

Thanks, this works great. I can see it's based on relesebranch_7_8, could you rebase it to master please?

However, this requires the creation of a new PR based on the master branch?

@petrasovaa petrasovaa changed the base branch from releasebranch_7_8 to master August 12, 2020 15:30
@petrasovaa petrasovaa changed the base branch from master to releasebranch_7_8 August 12, 2020 15:30
@petrasovaa petrasovaa merged commit 90fc283 into OSGeo:releasebranch_7_8 Aug 12, 2020
petrasovaa pushed a commit that referenced this pull request Aug 12, 2020
@tmszi @petrasovaa
wxGUI/animation: add export output animation file validation, before …
daa644f 
…export file (#839)
@petrasovaa
Copy link
Contributor

Thanks, this works great. I can see it's based on relesebranch_7_8, could you rebase it to master please?

However, this requires the creation of a new PR based on the master branch?

Hm, not quite sure how this works on Github, I merged this for now to 78 and cherry-picked to master.

@tmszi tmszi deleted the fix_g_gui_anim_check_exp_file_postfix branch August 12, 2020 16:09
@veroandreo veroandreo mentioned this pull request Aug 14, 2020
Merged
@neteler
Copy link
Member

neteler commented Aug 22, 2020

Shall this be backported?

@petrasovaa
Copy link
Contributor

See comment above, it's in both branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrasovaa petrasovaa petrasovaa requested changes

@veroandreo veroandreo Awaiting requested review from veroandreo

Assignees
No one assigned
Labels
GUI wxGUI related
Projects
None yet
Milestone
7.8.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tmszi @neteler @petrasovaa