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

Register Views before creating them #3181

Merged
merged 23 commits into from
Dec 8, 2021

Conversation

akavel
Copy link
Contributor

@akavel akavel commented Dec 6, 2021

Pull Request Description

Make sure components implementing the View trait are registered in view::Registry, so that their keyboard shortcuts can be displayed to the user in a help window immediately after the IDE is started.

Fixes #3093.

Important Notes

  • components that did not emit warnings, but still implement the View trait, were also tracked and case-by-case decisions taken to make sure appropriate registering behavior is performed; in particular:
    • FloatLabel was modified to not implement View, as it's a private widget and not expected to need keyboard shortcuts
    • NumberPicker & NumberRangePicker were registered
    • Scrollbar was skipped for now as it's understood to not be complete & ready to use yet
    • TopButtons was registered (tested with ?is_in_cloud=true)
    • CloseButton was modified to not implement View (the widget seems also internal: its creation was actually not even via new_view; it was used in 2 separate specializations & places as "close button" and "fullscreen button"; it had no keyboard shortcuts; a decision as of now was made to de-View it, with a possibility to reconsider in future if the understanding of its situation changes/develops)

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

It's an internal, private component/widget; not expected to have shortcuts
The component is currently presumed to be an internal widget, and
doesn't have keyboard shortcuts. Also, its use is currently not well
aligned with the claimed "CloseButton" label, as it's also used in
function of a "fullscreen button". The decision can be revisited in
future if a different understanding is reached.
@akavel akavel marked this pull request as ready for review December 7, 2021 13:20
Comment on lines 470 to 471
// NOTE: if implementing a different specialization of ListView, make sure to change the label
// appropriately so that 'register' would properly differentiate shortcuts etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this comment is right. First, I'm not sure what is a different specialization - Rust does not have specializations in c++ style. And the entry does not affect the way the shortcut are handled, so it fact each ListView should have the same label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Now that I think of it, if guidelines should be provided anywhere, I'd rather expect to see them in View trait's docs, forming a part of the trait's contract description. With that said, I'm more than happy to remove it from here. It's not however clear to me what are the rules around the various methods of View, so I don't currently feel confident to be adding docs to View.

Do you think it'd be ok for me to just remove this comment here, or would you suggest I try adding comments in View, or somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with just removing it.

@akavel akavel requested a review from farmaazon December 7, 2021 14:50
@4e6 4e6 merged commit 5e5abac into develop Dec 8, 2021
@4e6 4e6 deleted the wip/akavel/view-registration-warning-180504109 branch December 8, 2021 07:11
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.

Warning on startup: view_registry complaining about views being created but not registered
4 participants