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 #19703, make filter translatable and add NetCDF #8245

Merged
merged 4 commits into from
Oct 22, 2018
Merged

Fix #19703, make filter translatable and add NetCDF #8245

merged 4 commits into from
Oct 22, 2018

Conversation

rduivenvoorde
Copy link
Contributor

Description

This fixes https://issues.qgis.org/issues/19703 where files without an extension were not show in the Open Mesh dialog.
Additionally the filter string is translatable.
And I added NetCDF (*.nc) files to filter list (which is working for me here, but beter @PeterPetrik checks if this is really the case).

Checklist

  • [ X ] Commit messages are descriptive and explain the rationale for changes
  • [ X ] Commits which fix bugs include fixes #11111 in the commit message next to the description
  • [ X ] Commits which add new features are tagged with [FEATURE] in the commit message
  • [ X ] 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
  • [ X ] 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
  • [ X ] I have run the scripts/prepare-commit.sh script before each commit

@rduivenvoorde
Copy link
Contributor Author

Note that grepping for All files (*.*) that there are still a lot of places where this is used, while according to: http://doc.qt.io/qt-5/qfiledialog.html#setNameFilters this is not portable and should be All files ( * ).
Should we fix them all?
OR should we create (at least for the 'All files'-case) some constant?

Also it looks like for the qfiledialog the preferred way to filter is not the extension anymore but the 'mimetype filters'. Not sure what happens when you set both.
Also not sure if ALL filetypes have such mimetype (was suprised that at least NetCDF has one: https://en.wikipedia.org/wiki/NetCDF (application/netcdf and application/x-netcdf)

I was not sure if I had to put it on the end of the filters, if I'm right there is a convention that on OPENING dialogs the All Files filter is to be put as first, while in SAVING it is put as last (and the first one is the preferred format). Right?

Also wondering about the translatability. I just put the whole file filter in the tr() function, while I also see places where a dev split up the filter string in parts and then choose to make those translatable or not.

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Shouldn't there be a space before each '('?

@rduivenvoorde
Copy link
Contributor Author

rduivenvoorde commented Oct 20, 2018

@nyalldawson yep you are right, added. And also had a look at the Apple HIG guidelines on how combobox text labels should be capitalized: https://developer.apple.com/design/human-interface-guidelines/macos/fields-and-labels/combo-boxes/ So also used Title Case for the other strings.

Notes for > 3.4 (?) would be nice to do a mass edit of all the filters in QGIS, to:

  • add a space to all filter combo boxes in QGIS (on gnome you do not see the filter-regexp, but on Windows you see the literal string), now it is a mix of all
  • use "All Files ()" instead of non portable "All Files (.*)"
  • make all filter labels Title Case (now it is both 'All files' and 'All Files')
  • make them good translatable: find out if splitting the name (IS translatable) and the filter-regexp (not translatable?) is preferred OR not (how do chinese/japanese see filenames on their systems??)

@PeterPetrik PeterPetrik self-assigned this Oct 22, 2018
@PeterPetrik PeterPetrik merged commit 4fa62e9 into qgis:master Oct 22, 2018
@PeterPetrik PeterPetrik self-requested a review October 22, 2018 06:18
@PeterPetrik
Copy link
Contributor

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants