Closed Superkat32 closed 4 months ago
Gave this a test and thought at first it was broken. Despite knowing that there should be a colour picker, I did not understand that I had to click the small colour field to invoke it. Maybe it should always open when selecting it with the mouse?
Also there's a small bug: after you open the picker, you cannot use the keyboard to change the values until I click anywhere (though it does not appear to matter where). After one click, both mouse and keyboard work fine in any order.
Gave this a test and thought at first it was broken. Despite knowing that there should be a colour picker, I did not understand that I had to click the small colour field to invoke it. Maybe it should always open when selecting it with the mouse?
I say let's see what Xander thinks about changing it. I don't mind the idea of having it always open upon clicking the color controller.
Also there's a small bug: after you open the picker, you cannot use the keyboard to change the values until I click anywhere (though it does not appear to matter where). After one click, both mouse and keyboard work fine in any order.
I'll look into it, thanks.
I've made a lot of the requested changes, including remove unneeded variables, renaming methods to fit your naming scheme, fixing the keyboard inputs on the color picker, and removing the unused access wideners.
I've also tried to make all the popup color picker related stuff to a popup controller widget. I ask that you look at this because I'm not feeling super confident about it, but I can't figure out what I'm not feeling confident on. I tried sleeping on it but that didn't help either.
I also ask that you consider this feedback and see if I need to change anything.
Gave this a test and thought at first it was broken. Despite knowing that there should be a colour picker, I did not understand that I had to click the small colour field to invoke it. Maybe it should always open when selecting it with the mouse?
How about you flash the outline from black to white and back again slowly when hovering, as if to say something would happen if you click it.
I am not very good with Java generics, so I don't know if this actually poses a problem or if there's an easy solution. Maybe @isXander can weigh in:
The new ControllerPopupWidget
extends ControllerWidget<Controller<?>>
. But if I have a popup over an element inheriting from StringControllerElement
(which in turn extends ControllerWidget<IStringController<?>>
), I don't believe that can be plugged into that, right? I'm asking because the dropdown controller could make good use of this "popup" facility, but I cannot see a way to cleanly add the two together.
Can this be made work by templatifying ControllerPopupWidget
?
I am not very good with Java generics, so I don't know if this actually poses a problem or if there's an easy solution. Maybe @isXander can weigh in:
The new
ControllerPopupWidget
extendsControllerWidget<Controller<?>>
. But if I have a popup over an element inheriting fromStringControllerElement
(which in turn extendsControllerWidget<IStringController<?>>
), I don't believe that can be plugged into that, right? I'm asking because the dropdown controller could make good use of this "popup" facility, but I cannot see a way to cleanly add the two together.Can this be made work by templatifying
ControllerPopupWidget
?
I believe you can do ControllerWidget<? extends Controller<?>>
Ah yes, public abstract class ControllerPopupWidget<T extends Controller<?>> extends ControllerWidget<T> implements GuiEventListener
seems to work
Is this ready for review?
Is this ready for review?
I believe the only thing I'm waiting on now is for you to add the config for the color preview outline to know if it should be flashing or not. I was going to ping you about this on the weekend but you beat me to it :) /lh
I've resolved the final comments(took me a little bit because I wanted to give them a good look before resolving). If any other changes are needed on my end, please let me know!
I found out there is a request re-review button :) I don't believe any other changes are needed unless someone finds some code that could be improved or changed.
Finally!
Streamline the color config option by more easily allowing users to choose colors. With support for color controllers that have alpha enabled as well!
Other details:
The color picker works by detecting if a user has their mouse down in the color preview of the color controller. The color picker widget is a custom screen that will render the YACLScreen in its
renderBackground
method. Upon the window being resized, or the color picker screen being closed, the minecraft client screen will be set back to the YACLScreen without reinitializing it. ThePopupColorPickerScreen
doesn't extend thePoupScreen
as I figured it would be better to have it this way(also I was messing something up somewhere because it didn't let me extend it)I ask that you look over everything just to make sure that it is okay. I'd also like to have you specifically look at method I added in the
ElementListWidgetExt
class. I added a method to get the color picker's y with smooth scroll, as I couldn't determine if there was a better way of doing so.There are also a LOT of commits in this PR. You only need to worry about the last 7-ish, as code in others were either replaced/redone, or were checkpoints for mechanics that were replaced/redone. I don't mind copy-pasting my work into a new fork to have just one commit if you care about commit history.
Beyond that, I also ask for any feedback you may have, as I've never done a PR this big before.