Move classify related wigdets in dialogs #4660
Conversation
+1. I've always found this setting confusing. Perhaps adding a detailed tooltip explaining exactly what it does would also help? |
@nyalldawson the tooltip is there already. It was how I was able to understand what that was for. Tooltip: If checked, any pixels with a value out of range will not be rendered |
|
@SrNetoChan , good initiative. If you do that, IMHO, you'll also have to update the vector's graduated and categorized renderer to insure our UI is consistent there (i.e., both graduated & categorized renderers have the [ classify ] button below the list widget). Also, we shouldn't put mode, classes, and the classify button on the same line, it'll hurt users who like to have a narrow style dock panel. You could either have classification mode on its own line, and classes + classify on a row below it, or possibly put the classes spinbox or classify button on the label unit prefix row (the latter would save vertical space, but may not be the greatest regroupment of widgets 😄 ) |
|
Re the width issue, also keep in mind that labels are localized into other languages and might be longer than the width in English (hence why I'm reluctant to have three options on one row). |
|
Also keep in mind that the paletted/unique renderer has a similar button, which will also need to be moved. |
|
@nirvn thanks for your inputs. I will change the other dialogs to make them consistent. But before I do it, I would like to present some alternatives so that you guys could give me your opinion. Alternative 1. Everything under Color ramp
This is my favorite. It still allows to make very narrow dialog in docker mode, and it is clear that all widgets belong to the same functionality. Alternative 2. This tries to make the widget more in line with the rest the dialog
Alternative 3. Same as before, also including the classes
**Very weird composition as you predicted ** :-P
|
SrNetoChan
changed the title
SrNetoChan
changed the title
|
I changed all dialogs with classify. @nirvn @nyalldawson let me know what you think. I am still not sure about indenting the the widgets or not. Singleband pseudocolor
Graduated by size
Graduated by color
Categorized
|
|
I'm not convinced on the indented widgets either - seems like a lot of wasted space to me, in dialogs which need to be very space efficient to work well in dock mode. Can you make sure that any change you make to the vector categorized widget is propagated to the unique/paletted raster widget too? That's been designed to mimic the same functionality as the categorized renderer so it'd be good to keep the widgets with similar UI. |
For categorized vectors (and all the others), we use a "Classify" button to populate all missing values. Would you oppose to use the same name in the raster palleted/unique one? I know it's not exactly the same, but it would add consistency to the dialogs, and the tooltip explains everything quite well. Besides, would it be ok to change the "load from layer" text by an icon? In single band pseudocolor, we use this icon and tooltip to do a similar operation.
|
|
I removed indentation for "mode". Meanwhile I realized that "Classify" button makes sense to be near the "add" and "remove buttons" as it as kinda the same functionality (it's an "add all values" button). I also added some tooltips to the buttons.
I plan to propose some changes to the unique/paletted raster widgets to be more in line with these dialog, but I will prepare another PR for that. |
SrNetoChan
force-pushed
the
pseudocolor_dialog
branch
from
d2640fb
to
52fa7d9
Compare
|
@nyalldawson @nirvn I know you guys are pretty busy trying to submit your own work before the feature freeze, but If you have a couple of minutes to look into this, I would appreciate it. thanks! although I am not sure what is travis compaining about. |
|
@nyalldawson @nirvn All tests are now passing. Can any of you review this please? Thanks! |
|
@nyalldawson @nirvn Do you guys think this PR is an interesting change? or should we just close it? Thanks! |
|
I think we should move forward with this. @SrNetoChan Can you summarise the current situation? Does this PR still apply? |
|
@nyalldawson I think it does. It's has been a long time since I have touched this. I will pull the recent master and see how it looks. Since we are already in feature freeze, I will also implement the palleted unique values dialog. |
@SrNetoChan does this target 3.4 in this case and is not a simple UI fix that would make sense for 3.2 already? |
SrNetoChan
force-pushed
the
pseudocolor_dialog
branch
from
8a1f46d
to
9eded9c
Compare
|
@nyalldawson @m-kuhn to summarize the situation of the pull request. The PR tries to improve the UX when using the vector categorized, graduated renderer dialogs, and the raster singleband pseudocolor, (and eventually palleted colors). The changes are only at the UI level. Besides adding or improving some of the tooltips, the interpolation mode has been move from below the "values/class/range list" to near the interpolation method. At some point I was also proposing to move the classify button above, but I think it is in the right place. Vector graduated by color
Vector gradauted by size
Raster singleband pseudocolor
Finally, I have changed the palleted/unique values widget to be more in line with the vector categorized and gradutaed widgets.
With that in mind, I wonder if all the buttons (reload from band, load from file, save to file) in the singleband widget shouldn't move to an "Advanced" dropdown menu as well. But this is not implemented.
|
|
What do you think about putting the classification parameters into a "Classify" group box, together with the "Classify" button? I think they contextually belong together and should somehow be separated from the rest of the configuration options. |
|
@m-kuhn I am not sure about that. They do belong together, but they are also related to the Values table. Isolating them in a group may be more confusing. At some point I did join all the widgets in the same place, but then I came to the conclusion that the Classify button belongs to the button area. After all, the classify button is a "Add all values" or a "reload values from data" button. |
|
Do these fields change anything as long as the classify button is not clicked? |
|
@m-kuhn I don't think so. But I would need to test. |
|
For me, if widgets and a button are so closely tied together in their functionality, it makes sense to put them next to each other and away from other configuration elements. In the current proposal I get the feeling that "Mode: Equal Interval" is a configuration option just like "Size: from 1 to 2 mm". |
|
My favorite would probably be to put them where they were before at the bottom, put the Classify button next to the widgets (on the right), relabel it as "Reclassify" to make it clear it will delete current classes (it does IIRC) and maybe put it in a group box with the label "Reclassify". |
But that's the thing. The mode is related to the size interval (if one choose size as the Method), the coloramp (if method is color), the interpolation, and even the legend. It's when you click classify that all those options are put in use to fill the table.
We would need to put everything inside that Reclassify group, because all those options are only used when you click the classify button (except for the column name in vector layer that you must set anyway). If others prefer, I would not mind moving the classify button to the top of the table again (one of the earlier proposals). That would put everything together and might avoid people hitting the classify button by mistake. But putting it all in a group is a bit of overkill to me. About the reclassify text, if my skill was good enough I would change it after the first use from "Classify" to "Reclassify". I think I can try that. |
For me this sounds like a good proposal, pinging @nirvn for his always useful inputs conerning UX |
That's not strictly true - changing the color ramp takes affect immediately without requiring reclassification |
|
Ping @probot |
|
I tried to extract color ramp editor to separate widget in the PR: #7313 to be used for styling contours in mesh layers. It looks like we should cooperate these two initiatives? |
|
@PeterPetrik Do you agree about moving the classify button up as well? |
|
@SrNetoChan I do not have a preference, just I wanted to note that I extracted color ramp shader widget https://github.com/PeterPetrik/QGIS/blob/mesh_layer_styling_gui/src/gui/raster/qgscolorrampshaderwidget.h in the PR and as long as it is usable for both mesh and raster layers it is ok for me. Also to note, I did not used it back in raster layer styling yet, since it is not merged and I wanted to do it in 2 steps. But I noticed this PR and it looks like it should be coordinated :) |
remove unneccessary columns
SrNetoChan
force-pushed
the
pseudocolor_dialog
branch
from
9eded9c
to
90ed1e3
Compare
|
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
|
stale
bot
added
the
stale
|
The build is failing after I moved the classify button up. Anyone knows
what I need to do to fix it?
A qui, 12/07/2018, 17:21, stale[bot] <notifications@github.com> escreveu:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4660 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADcKeYOqdBhI3D1ApcFGuZD0XtL1K5Zsks5uF3eDgaJpZM4NsU1c>
.
--
Alexandre Neto
---------------------
@AlexNetoGeo
http://sigsemgrilhetas.wordpress.com
http://gisunchained.wordpress.com
|
stale
bot
removed
the
stale
|
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
|
stale
bot
added
the
stale
|
While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist. |
Description
Moves Classify related widgets next to the Color Ramp.
IMHO, classify has nothing to do with the Add, Remove, load... buttons. and it's totally related to the Mode and classes widgets, so it should be in the same line. Assuming that the user's workflow steps will be: set the mode; choose the number of classes; and hit Classify; I decided for the right side.
Besides, these three widgets are also related to the color ramp, so I move them next to it. I would have preferred to keep the three widgets below the color ramp selector (excluding the label) but in Docker mode, it would be too large.
I also prefer that the Add, Remove, etc... buttons are as close as possible to the tree widget they are related to. and this way they are.
Changes Clip out of range values text
The text "Clip out of range values" sounded unclear to me, as for me clipping is associated to cut off a part. I propose change it to "Hide out of range values". Alternatively it could be "Don't show out of range values"
Screenshots
Current state
Proposal
Checklist
fixes #11111in the commit message next to the description[FEATURE]in the commit message[needs-docs]in the commit message and containt sufficient information in the commit message to be documentedscripts/prepare-commit.shscript before each commit