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

Added ability to set default value(s) for UISelectionList #213

Merged
merged 17 commits into from Aug 27, 2021

Conversation

teaguejt
Copy link
Contributor

This commit adds a default_selection arg to the UISelectionList constructor that sets the list's default state. It supports multiple defaults for multi-selection lists.

I'm not sure how relevant screenshots are since it's just a list with some items selected, but I'm more than happy to provide some if you'd like.

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2021

This pull request introduces 4 alerts when merging d428d50 into 1caf11b - view on LGTM.com

new alerts:

  • 4 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2021

This pull request introduces 4 alerts when merging da6cb6d into 1caf11b - view on LGTM.com

new alerts:

  • 4 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2021

This pull request introduces 4 alerts when merging 9599861 into 1caf11b - view on LGTM.com

new alerts:

  • 4 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2021

This pull request introduces 4 alerts when merging ebcb703 into 1caf11b - view on LGTM.com

new alerts:

  • 4 for Unused import

@MyreMylar
Copy link
Owner

Hello!

Thanks for writing this, I've just had a quick read through of the changes to get a handle on them, and it has been a fair while since I've looked over this codebase now so I may have missed something.

My initial impressions is that the changes to the selection list seem fine, I understand the motivation for wanting default items selected in there, I guess I didn't add it originally because the selection list grew out of the drop down/combo box menu which already has a default display built into it as the other non-selection list part of the drop down.

I haven't compiled and tested it yet but overall it seems fine on a line by line read.

I'm slightly less certain on the toggle button implementation so far. It looks like you are using the 'active' button state to represent a toggled on/off button. Should this be the selected state instead? My feeling is that would be more consistent with the way the buttons are used in the selection list where they are effectively toggled as selected or not. I'm not 100%, the names I've given to UI states probably are not helpful, I was never a profession UI designer/developer so I was making a lot of it up as I went along :)

What seems more certain to me is that you could probably implement the toggle button with a little less code duplication from the UIButton. It looks like most toggle buttons only actually toggle on a mouse up (rather than a mouse down). This means you could use the already existing self.pressed_event variable when you extend the `process_event() method. Something like:

def process_event(self, event: pygame.event.Event) -> bool:
    consumed_event = super().process_event(event)
    if self.pressed_event:
        # button pressed, toggle it
        if self._state:
            self.select()
        else:
            self.unselect()

This kind of thing just helps reduce code duplication so later when I, or someone else, inevitable has to rip up the button event code it is largely only in one place.

I also suspect that the library as a whole should probably allow hovering of selected buttons, but I can't remember why I disallowed it, there may have been a good reason or it may have just been me moving quickly while working on something else.

Anyway if it turns out there was no good reason, we can turn that on turned on in the can_hover() method of the UIButton and then make _get_appropriate_state_name() (which needs a better name) be overridden in UIButton() to fix the unhovering state so it properly goes back to selected. I think the base UIButton enable() should also be updated to better handle the selected state, right now if you disable and re-enable the UI it clears out the selection from a selection list which I don't think is ideal behaviour.

@MyreMylar
Copy link
Owner

Also, there are those unused imports that the lgtm bot is pointing out.

@teaguejt
Copy link
Contributor Author

Whoops, apologies about the toggle button. That was supposed to be in a local dev branch. I really only intended to ask to merge f880492. Let me fix the imports and jump back to where it's just the selection list defaults.

The toggle button can be implemented more cleanly, as you suggested, but I haven't had the time to focus on it as much later in the week.

@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2021

This pull request introduces 5 alerts when merging a3c78f0 into 1caf11b - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for Unused local variable

Copy link
Owner

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Alright, looks good!

@MyreMylar MyreMylar merged commit 032625d into MyreMylar:main Aug 27, 2021
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

2 participants