Added disabled variations for most UI graphic #2521
Conversation
|
I'd ask @Modanung about changes in UI.png since he is the author of this texture. |
|
The SourceAssets folder contains the SVG version of the UI. It would probably be better to modify that file instead and export it to create the new PNG in order to to keep things synchronized. |
|
Ah cool, I was not aware. I'll take care of that in a while, may be on ice a couple of days or week(s?). |
|
Would be nice if you can check that the file looks good. I've never edited a svg before. I guess it might also be nice to squish the two commits together or so (since I committed a new export to replace the manually edited one and there's not much point in keeping that bit of history), but I'm still not too clear about how it works. |
|
I am not struck by any oddities in the SVG. There's another image file I forgot to mention: The UITemplate.xcf |
|
Hm, that looks like quite a bit more work. Is it used for something now (the pixel positions I guess, but not in any automated fashion as far as I can tell)? Seems like it covers kinda the same thing as the svg. I'll do it if you think it's necessary but it seems like kind of a pain. |
|
I don’t think that GIMP file is necessary, definitely not to be updated manually. |
|
Stale pull request message |
weitjong
added
backlog
улучшение
|
I realized one issue with this, which is that non-enabled in ui is the same thing as "passes through interaction", so for example scrollers end up always showing the movable bar as it's disabled graphic, because they are borderimages that are left disabled so that the interaction goes to the scroller (the scroller sets the child object to highlighted when it itself is). Imo this feels really off, but it is how things work at the moment. One idea could be to make enabled into a enum with enabled, disabled and passthrough as values, but what do you think? |
|
After some consideration I think it would probably be a better idea to add a new flag to set the disabled graphic, reason being that it's easier to handle the serialization that way. I'm thinking "active" (so SetActive, IsActive etc), cause I believe it's not used for anything else in UI. The interaction would then be: if !active and enabled then display disabled graphic and eat the input, if !active and !enabled display disabled graphic and ignore input, if active then handle as before relative to enabled flag. |
|
This commit should have almost no effect on current code (since active is default), with the exception that Button (which was the only element type that had disabled graphics capability before) will now not display the disabled graphic on !enabled, but when !active (like other types). |
|
I agree that we may need the second flag, but the current semantics seem too confusing for me. If "enabled" is false, then UI element shall be displayed with disabled graphics and it shall ignore input. So, if you want to add some new flag, the aforementioned behavior of "enable" flag have to be preserved because this is what people expect from "enable" flag. https://doc.qt.io/qt-5/qwidget.html#enabled-prop |
|
Speaking of initial problem...
|
|
I think scrollbars are pretty much it. I guess it could alternatively be done by adding like a "passthrough" flag that's true per default, to split that behavior from enabled. The reason why I went with the "active" idea was because it leaves things mostly the same as long as you don't change the new variable. I do agree that enable doesn't quite work as expected at the moment, or I should perhaps say that it's not used as expected. UIElements have enable set to default false because it means passing thought the input, but you would as you say expect that to mean something it really doesn't (default graphics for types other than buttons is actually also a relatively new thing that I added in a previous pr). Anyway, what do you think? |
|
I actually did a version that changed enabled to an enum with passthrough, enabled and disabled initially, but I realized I wasn't really sure how to make it compatible to synchronize. I guess it is an option though, although it might look more messy. I think I would recommend the passthrough flag if active is no good. |
|
Assuming we want "enabled" flag to have sane behaviour conisistent with all other UI libraries... "Enabled" flag controls the skin of the element (unconditionally). Both "Enabled" and "Passtrough" are ON by default, so the generic UIElement behaves just like before.
Do you see any issues with this approach? |
|
That's pretty much exactly what I had in mind, sounds good. I'll implement it next week. |
This reverts commit fa050aa.
| @@ -42,7 +42,6 @@ ProgressBar::ProgressBar(Context * context) : | |||
| value_(0.0f), | |||
| showPercentText_(true) | |||
| { | |||
| SetEnabled(false); | |||
There was a problem hiding this comment.
Bit unsure about this one. It might have been meant to be true (since it changes the default below), in which case there should be a SetPassthrough(false) here.
|
Now it's starting to look good, even tho I still have some questions about implementation details. I need to look into code a bit more. One of which is that I'm not 100% convinced we need deep passthrough. |
|
Well, passthrough and enable work very similarly as far as input is concerned, so I suspect it probably does need it. That said, I think that the user will basically almost never manipulate passthrough, and that it'll mostly just be set once (and set their defaults) by UIElement subtypes. |
|
I looked at the code a bit more and I changed my mind. Deep passthrough is totally fine. Here are my issues.
This is so much shorter than current version of code.
It has to be extracted into function. |
|
Mm, most of the issues are cause of copy-paste of existing stuff (except 3 and 4, which I didn't think about but that's reasonable). Anyway, I work on stuff related to Urho3D on the second half of the week. I'll fix it then, unless you do it before that which is fine too. |
|
Do you think IsInputEnabled should check for visible too? Some places do that though not all, not sure if that's just left out by mistake more or less. |
|
Speaking of places that don't check for visibility... |
|
Hmm, probably not cause it does check in GetElementAt (which I believe is used in the input process to find the element to interact with?), but not 100% sure. Ed.: There is some stuff with various BringToFront things that disallow disabled but ignore visible at the moment I think. |
| } | ||
|
|
||
| void UIElement::SetDeepEnabled(bool enable) | ||
| { | ||
| enabled_ = enable; | ||
|
|
||
| for (Vector<SharedPtr<UIElement> >::ConstIterator i = children_.Begin(); i != children_.End(); ++i) | ||
| (*i)->SetDeepEnabled(enable); | ||
| for (SharedPtr<UIElement> child : children_) |
There was a problem hiding this comment.
It should be either const SharedPtr<UIElement>& child or UIElement* child, since copying of SharedPtr is relatively slow operation.
I know that performance impact is negligibly small (since this is user API of UI subsystem), but it doesn't mean we should do premature pessimization when alernatives exist.
|
All in all, this looks good for me. |
|
Just a note about this PR. I think it's good to merge. Please keep this PR open, and it will be merged as soon as new stable version of Urho is introduced. |
No description provided.