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

Add "Create categorized renderer from styles" algorithm #2988

Merged
merged 3 commits into from Nov 8, 2018

Conversation

DelazJ
Copy link
Collaborator

@DelazJ DelazJ commented Sep 11, 2018

Fixes #2985

The specified expression (or field name) is used to create categories for the
renderer. A category will be created for each unique value within the layer.
Each category is individually matched to the symbols which exist within
the specified QGIS XML style database. Whenever a matching symbol name is found,
Copy link
Member

Choose a reason for hiding this comment

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

Does "XML" word is important for the final user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe to say that SLD does not work? Or maybe PG saved style?
For reference, I copy pasted almost the GUI

Copy link
Member

Choose a reason for hiding this comment

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

PG saved style is in XML :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right 😄 Remains the SLD based style that is not supported.

Copy link
Member

Choose a reason for hiding this comment

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

Again, please don't call the style a database... QGIS styles XML file?

Note that, for what I can see, you can't use XML stored in a PG database. This only works with XML files created using the Symbols Library export functionality. I would suggest we state this clearly. In the begining of reviewing this I was thinking in QML styles and SLD and styles stores in Postgres and geodatabases. But I don't think that's the way it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the word database is used in the user interface, it is natural that it is also used in the documentation. The user interface should be fixed before the documentation is changed.
I don't think it is a problem to include XML, since that is the file type that comes up as a default in the dialogue.

the specified QGIS XML style database. Whenever a matching symbol name is found,
the category's symbol will be set to this matched symbol.
The matching is case-insensitive by default, but can be made case-sensitive
if required.
Copy link
Member

Choose a reason for hiding this comment

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

how user can enable this?

Copy link
Member

Choose a reason for hiding this comment

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

Just add see below or so is enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it worth? This is a general overview of the capabilities of the alg. User is expected to read the parameters description later on. Also, I'm not sure we add this kind of reference in the other detailed algs. I'd be -0,5 ;)

Copy link
Member

Choose a reason for hiding this comment

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

My first comment is when reading the text, I though howto do this? Then I continue to read and then see the parameter list. My personal experience reading documentation is that you don't know if (and whrere) this kind ofthing will be documented.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @DelazJ and I would actually go even deeper and would remove from line 27 to line 35. this all can summarized in the parameters. Remember, less text written, less text we need to maintain :-D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed lines but kept lines 34-35 as they are about the output of the alg. Might be worth known in the alg description.

Copy link
Member

@SrNetoChan SrNetoChan left a comment

Choose a reason for hiding this comment

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

I think a clarification needs to be done about what kind of Styles file we are talking about. Other than that, is just subjective and cosmetic changes.

Create categorized renderer from styles |34|
--------------------------------------------
Sets a vector layer's renderer to a categorized renderer using matching symbols
from a style database. If no style file is specified, symbols from the user's
Copy link
Member

Choose a reason for hiding this comment

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

Can we try to avoid the word "database" in here. An XML file is not a database (although I know that one name of the parameters). Maybe "a styles XML file".

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the word database is used in the user interface, it is natural that it is also used in the documentation. The user interface should be fixed before the documentation is changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@havatv I think we can find the right wording here and issue a PR for the interface. Have been done many times in the past.
@SrNetoChan For me database is not that wrong; a db is not about a particular format here but should be read as a container that stores some data in some formalised structure. Also note that symbols used are not only from a provided file but also from the qgis installation (i think it's in a sqlite db format).
I'm however questioned by the use of style vs symbol word

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @DelazJ. Database is fine here.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the use of something like symbol collection, but I confess that the first comments I did I was confusing the type of input file. Now, I understand that the expected input file is an XML containing the description of one or more symbols/styles. something we
get from exporting symbols from the Style manager.

I agree with @DelazJ we can create a PR with the name we think is best and, once approved, change the documentation accordingly.

@DelazJ about the name style and symbol, I have already proposed the change of style manager to symbols manager once, but It was vetted by Nyall who has higher plans for it, which, according to him, will make the "style" term make sense for that dialog. For this algorithm, I think I would also prefer the use o the word symbol, since style in the layer context means labels, diagram, fields, etc...

from a style database. If no style file is specified, symbols from the user's
current style library are used instead.

The specified expression (or field name) is used to create categories for the
Copy link
Member

Choose a reason for hiding this comment

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

I would not say "field name", but just "field", after all, in the end the algorithm uses the field (values), not the name. I would remove the parenthesis also:

"The specified field or expression is used"

Copy link
Contributor

Choose a reason for hiding this comment

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

What you can supply here is the field name, but I agree that changing to "The specified field or expression is used" is better (or perhaps "The specified expression or field is used", since expression is used in the user interface).

The specified expression (or field name) is used to create categories for the
renderer. A category will be created for each unique value within the layer.
Each category is individually matched to the symbols which exist within
the specified QGIS XML style database. Whenever a matching symbol name is found,
Copy link
Member

Choose a reason for hiding this comment

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

Again, please don't call the style a database... QGIS styles XML file?

Note that, for what I can see, you can't use XML stored in a PG database. This only works with XML files created using the Symbols Library export functionality. I would suggest we state this clearly. In the begining of reviewing this I was thinking in QML styles and SLD and styles stores in Postgres and geodatabases. But I don't think that's the way it works.

the specified QGIS XML style database. Whenever a matching symbol name is found,
the category's symbol will be set to this matched symbol.
The matching is case-insensitive by default, but can be made case-sensitive
if required.
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @DelazJ and I would actually go even deeper and would remove from line 27 to line 35. this all can summarized in the parameters. Remember, less text written, less text we need to maintain :-D

Vector layer to apply a categorized style to.

``Categorize using expression`` [expression]
Field or rule of categorization of the features.
Copy link
Member

Choose a reason for hiding this comment

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

"Field or expression for categorization(...)"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or the other way around (Expression or field for categorization)

Copy link
Contributor

Choose a reason for hiding this comment

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

for categorization

Field or rule of categorization of the features.

``Style database (leave blank to use saved symbols)`` [file]
File containing the symbols to apply to the input layer categories.
Copy link
Member

Choose a reason for hiding this comment

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

maybe mention here that this file is an XML from a Symbols library export?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "File (XML) containing the symbols..."

Default: *False*

``Ignore non-alphanumeric characters while matching`` [boolean]
If checked, non-alphanumeric characters in style and category names will be
Copy link
Member

Choose a reason for hiding this comment

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

in the style and category names?

Optional

Lists all existing categories which could not be matched
to a symbol in the provided style.
Copy link
Member

Choose a reason for hiding this comment

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

just "List categories"?
to any symbol?
provided styles file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lists categories which could not be matched to any symbol in the provided styles database.

``Non-matching symbol names`` [table]
Optional

Lists all symbol names from the provided style file which could not match
Copy link
Member

Choose a reason for hiding this comment

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

just "Lists symbols..."?
provided styles file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lists symbol names from the provided styles database which could not be matched to any...

Copy link
Collaborator Author

@DelazJ DelazJ left a comment

Choose a reason for hiding this comment

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

A first commit to implement what seems to raise consensus in most of the comments... and more ;) Feel free to push commits to the PR or issue PR against the branch if you don't agree with the extra changes: I don't know whether I satisfy the database vs file, style vs symbol old "misuse" but I'd like to not spend more time on this alg (a bit of hassle).

Create categorized renderer from styles |34|
--------------------------------------------
Sets a vector layer's renderer to a categorized renderer using matching symbols
from a style database. If no style file is specified, symbols from the user's
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@havatv I think we can find the right wording here and issue a PR for the interface. Have been done many times in the past.
@SrNetoChan For me database is not that wrong; a db is not about a particular format here but should be read as a container that stores some data in some formalised structure. Also note that symbols used are not only from a provided file but also from the qgis installation (i think it's in a sqlite db format).
I'm however questioned by the use of style vs symbol word

the specified QGIS XML style database. Whenever a matching symbol name is found,
the category's symbol will be set to this matched symbol.
The matching is case-insensitive by default, but can be made case-sensitive
if required.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed lines but kept lines 34-35 as they are about the output of the alg. Might be worth known in the alg description.

@SrNetoChan
Copy link
Member

I was trying to submit a PR but it kept sending me to a 404 page on github so I gave up.

My proposal would be to add the following on line 40:

File (``.XML``) containing the symbols to apply to the input layer categories,
which can be obtained from the Style Manager :ref:`Share symbols <share_symbols>` tool.

Update according to input from @SrNetoChan
@havatv
Copy link
Contributor

havatv commented Nov 8, 2018

I tried an edit to resolve the last comment - sorry if that is inappropriate.

@DelazJ
Copy link
Collaborator Author

DelazJ commented Nov 8, 2018

It's more than appropriate @havatv We shouldn't let stale PR, particularly when no more is left to do. I had some concerns with Alex suggestion, reason why I did not update at that time (and forgot this PR) but i can't recall what the issue was.
Let's merge and if ever I remember and it's still valid, a new PR will surface.
Thanks

@DelazJ DelazJ merged commit 81163f4 into qgis:master Nov 8, 2018
@DelazJ DelazJ deleted the categorizedStyleAlg branch November 8, 2018 21:03
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

4 participants