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

fixed handling of input filenames with non-ascii characters [processing] #8968

Merged
merged 12 commits into from Feb 21, 2019
Merged

fixed handling of input filenames with non-ascii characters [processing] #8968

merged 12 commits into from Feb 21, 2019

Conversation

volaya
Copy link
Contributor

@volaya volaya commented Jan 24, 2019

Description

Fixed handling of input filenames with non-ascii characters.

Both raster and vector layers used as input for SAGA can now contain non-ascii chars.

Looks like there is no problem now in SAGA when using filenames with non-ascii chars, so some code used for replacing filenames has been removed. Also some code for python2, which is not needed now.

fixes #18617

It would be good if someone could test this in Linux before merging. Should work, but just in case SAGA behaves differently there.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@luipir
Copy link
Contributor

luipir commented Jan 24, 2019

Hi victor, can you check if there is already a test for out file with non-ascii chars? if not would be fine create a simple test... just duplicate one of the already available but set the output non-ascii... or create a unit test for that part of SagaAlgorithm.py
Just my 2c

@volaya
Copy link
Contributor Author

volaya commented Jan 24, 2019

Good catch, Luigi. Actually, I thought that both of the issues indicated in the comment where about the same (they are marked as possible duplicates), but they are not. One refers to inputs, and another to outputs. I fixed the inputs, but not the outputs.

Will redo the commit to remove the issue mention in there.

With outputs, it looks like SAGA can handle non-ascii chars, but then the file it generates has strange characters, so it wont be in the same path that the user selecteed, and Processing will show error.

Fixing that will require a different approach (although it requires this fix as well, to be able to write the script file), probably creating the file with a safe name and then renaming. Will work on that and create a separate PR with the corresponding tests.

Looks like there is no problem now in SAGA when using filenames with non-ascii chars, so some code used for replacing filenames has been removed. Also some code for python2, which is not needed now.

fixes #18617
@volaya volaya changed the title [processing] fixed handling of filenames with non-ascii characters [processing] fixed handling of input filenames with non-ascii characters Jan 24, 2019
@alexbruy
Copy link
Contributor

Looks good to me.

@@ -396,12 +396,6 @@ def exportRasterLayer(self, parameterName, layer):
else:
filename = os.path.basename(layer.source())

validChars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789:'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the Travis issue is stemming from allowing "." characters now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I added that code back, just in case

@nyalldawson nyalldawson added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Bugfix Needs Backporting labels Jan 24, 2019
@nyalldawson nyalldawson added this to the 3.6.0 milestone Jan 24, 2019
@volaya
Copy link
Contributor Author

volaya commented Jan 25, 2019

I added the code to handle non-ascii in outputs as well. Still missing a test, though

@volaya
Copy link
Contributor Author

volaya commented Jan 25, 2019

Tests added

@m-kuhn m-kuhn changed the title [processing] fixed handling of input filenames with non-ascii characters fixed handling of input filenames with non-ascii characters [processing] Jan 30, 2019
@stale
Copy link

stale bot commented Feb 14, 2019

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 14, 2019
@luipir
Copy link
Contributor

luipir commented Feb 14, 2019

@volaya any plan to fix the test?

@stale stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Feb 14, 2019
Copy link
Contributor

@luipir luipir left a comment

Choose a reason for hiding this comment

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

again approved

@luipir luipir merged commit 5a96fab into qgis:master Feb 21, 2019
@luipir luipir removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Feb 21, 2019
@luipir
Copy link
Contributor

luipir commented Feb 21, 2019

@volaya please backport soon... packaging will be tomorrow 22/02!

nyalldawson pushed a commit that referenced this pull request Jan 13, 2020
Fixes a regression bug accidentally introduced with 40134d6 (PR #8968) and backported with a887b7d (PR #9231): when there are multiple output parameters processAlgorithm incorrectly generates multiple saga_cmd commands, instead of a single command containing the output parameters.

Fixes #33658
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.

None yet

4 participants