search

StanfordLegion / prof-viewer

Legion Prof Viewer
Apache License 2.0
0 stars 5 forks source link

Allow ESC to reset UI #29

Closed bryevdv closed 1 year ago

bryevdv commented 1 year ago

This PR adds support for having the ESC key reset the UI. cc @lightsighter

Reset includes:

This PR also applies some clippy fixes related to or_default.

cc @elliottslaughter if there is some better approach that does not requires passing the windows list through, please let me know.

elliottslaughter commented 1 year ago

Most of this is fine, but:

clearing profile filter selections

To me it seems unintuitive that this would be controlled by ESC. It's a part of the control UI and not a pop-up window. And we don't touch any other part of the control panel (i.e., this does not implement a "reset to defaults" on the control panel).

What do other people think?

bryevdv commented 1 year ago

To me it seems unintuitive that this would be controlled by ESC.

I could see it going either way, but this was the explicit request from @lightsighter when I asked about the requirements (via slack)

elliottslaughter commented 1 year ago

@lightsighter can you clarify? I'm surprised that you want ESC to reset kind filters.

lightsighter commented 1 year ago

@lightsighter can you clarify? I'm surprised that you want ESC to reset kind filters.

I don't think I wanted it to reset the filters. I just wanted the pop-up box to go away. Although it might be nice if nothing is selected and you hit ESC then that clears the filters, but mainly what I want is for ESC to unselect anything I've selected and close any pop-up boxes.

bryevdv commented 1 year ago

👍 my misunderstanding, I've reverted that part in a2a5129 and also cleared the merge conflict.

elliottslaughter commented 1 year ago

Thanks for the clarification. I think it might be nice to have an option to reset things like filters, but I worry about having it be on ESC because it seems easy to press by accident. But we could potentially have a Reset button on the UI panel itself, or else come up with a key binding that's harder to press by accident (but then it'll be less intuitive as well, so maybe not a win overall).

Anyway, this looks good to me now, so I'll take it once CI passes. Please keep sending these kinds of UI feedback items if there is other stuff that we should consider.