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

Add keyboard shortcuts documentation to Viewer #2026

Conversation

juandejoya
Copy link
Contributor

Addressing issue #2011 with the creation of a popup button for helpful information and keyboard shortcuts. Help tooltips also included for all buttons except for selected geometry. Markdown doc also updated.

mtlx2011_helpui

Notes

  • Would we like to put in other information in the help menu? From what I've scanned in the doc, keyboard shortcuts seemed like the most appropriate (and matches what the Graph Editor is doing).

Copy link

linux-foundation-easycla bot commented Sep 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@juandejoya
Copy link
Contributor Author

Current CI build failures that I'm seeing are a result of missing reference. Will amend in next commit.

@kwokcb
Copy link
Contributor

kwokcb commented Sep 26, 2024

This looks great!

One thing I noticed is that "u" hotkey is not mentioned in the docs so I don't think yo have it here. It will toggle the display of the UI. Could you update the docs + help?

When it's hidden perhaps it would be useful to be able to still access help. I know it's beyond the original scope but maybe a "?" hotkey addition since you can't see the "Help" menu anymore. Purely a suggestion.

@juandejoya
Copy link
Contributor Author

Thank you, and sure! A single letter hotkey (H) might be better - the keyboard schema for symbols is going to be different depending on the country said keyboard is manufactured for.

@jstone-lucasfilm
Copy link
Member

This is a great start, thanks @juandejoya!

One layout change that I would recommend is to place this new button at the top of the Advanced Settings panel, rather than making it a top-level button, so that it doesn't impact the visibility of the viewport for users. This is particularly an issue for teams using the MaterialX Viewer in production, where viewport visibility is just as important as documentation.

Within the Advanced Settings panel, I might recommend naming the new button Keyboard Shortcuts for consistency, matching the title that you've placed at the top of the new panel.

@juandejoya
Copy link
Contributor Author

juandejoya commented Sep 26, 2024

Following @jstone-lucasfilm's feedback, we've changed the Help button to be a Keyboard Shortcuts toggle button that now lives at the top of the Advanced Settings pop up menu. Toggling the button should expand/collapse the shortcuts table. This should keep all the information in one popup and not crowd the screen.

Added the U shortcut for toggling the Viewer Options window and removed the additions to the Markdown doc as well.
mtlx2011_keyboardshorcuts_in_advancedsettings

I'm not sure whether @kwokcb's suggestion to add a shortcut to toggle the help menu is still useful if it's now nested under Advanced Settings. We could add a shortcut key for Advanced Settings instead, but I ran into a possible nanogui bug where a Popup's state is not updated when setting its visibility (investigation here). Not sure what's going on just yet and whether it's needed for the overall user experience, but happy to learn more from someone who may know if this is a legitimate bug or something that I may have overlooked in the API.

@jstone-lucasfilm
Copy link
Member

@juandejoya I really like this approach, where the help text expands into the Advanced Settings panel!

One minor suggestion that I'd make is to place the new Keyboard Shortcuts bellows at the bottom of the Advanced Settings panel rather than the top, so that it's "extending" the panel rather than "pushing down" the other options.

Does that change sound reasonable to you as well?

@juandejoya
Copy link
Contributor Author

Thanks! Sure, that suggestion makes sense - we do want the settings to be the first thing people see.

We could refactor the advanced settings code into bellows as well. It would be more readable and we can minimize the size of the Advanced Settings popup and expand it only as needed, which would help with visibility. It could look something like this mockup depending on how we want to restructure the settings:
mtlx2011_bellows

@jstone-lucasfilm
Copy link
Member

That's another compelling idea, @juandejoya, though we should probably reserve that for a separate PR following this one, so that we're not combining too many new ideas in a single change. Once this change is merged, I'd definitely be open to a PR that proposes a new structure for the Advanced Settings panel.

@juandejoya
Copy link
Contributor Author

Following @jstone-lucasfilm's feedback, moved the Keyboard Shortcuts button to the bottom. This requires a small change in the layout for Advanced Settings. Included setting the scroll position in the Keyboard Shorcuts button's callback function for usability.
mtlx2011_move_shortcuts_to_bottom

@kwokcb
Copy link
Contributor

kwokcb commented Sep 27, 2024

Hi @juandejoya, @jstone-lucasfilm

  • IMHO it feels a bit strange to have help under advanced options as my assumption is help should be easy to access as with the GraphEditor (which is top level UI).

  • Some ideas:

    • My initial thoughts where to add a icon button to the "View Options" title bar -- but I don't want to derail things and cause undue work.
    • Have the help key short cut. If this is too much trouble due to restrictions then don't bother but something quick to access would be nice.
  • Minor thing: From the video, if it's in the advanced UI, I'd suggest having a title (say "Help") to delineate the keyboard shortcuts area as it looks like it's the shortcuts are part of "wedge rendering" due to how it's indented.

@juandejoya
Copy link
Contributor Author

Thanks for the feedback @kwokcb. No worries about undue work - it's easy to adjust and we should commit to something that's best for a user.

The main issue with the original prototype is that the screen space coverage hinders visibility. What we're doing right now addresses that, but if we do revert back to a top-level button where the pop-up has the same (if not smaller) dimensions it would effectively have the same effect while being more discoverable. The benefit I see with having these under Advanced Settings is consolidation, but in retrospect it's probably better to just have an immediate place where people can look for help.

What do you think @jstone-lucasfilm?

I'll follow up on the help key shortcut key and maybe ping someone in the project slack if they have had any experience with nanogui's popups.

@kwokcb
Copy link
Contributor

kwokcb commented Sep 27, 2024

I chatted with @jstone-lucasfilm off-line. Just ignore my comments and use the current design.
Sorry for the unnecessary churn.

@juandejoya
Copy link
Contributor Author

No worries at all, and good to know! I'll probably ask offline about the design tradeoffs, mostly just to learn what about the pros/cons for future work.

@jstone-lucasfilm
Copy link
Member

@juandejoya The latest version is looking really good, and I like @kwokcb's suggestion to add a label just above the new Keyboard Shortcuts bellows, so that it has a distinct section within the Advanced Settings panel. Perhaps the title of this label could be Documentation, opening up the possibility of additional documentation bellows being added in the future?

Modify layouts so we can add the Documentation label without additional margins/indents.
@juandejoya
Copy link
Contributor Author

Changed the method name to reflect that we're creating an interface for Documentation rather than just Keyboard Shortcuts in Advanced Settings. We had to modify the layout to account for the Documentation label as well.
mtlx2011_createDocumentationInterface

@jstone-lucasfilm
Copy link
Member

This looks great to me, @juandejoya, and once we've had additional eyes on the details of the change, I think we should be good to merge this work.

Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Let's hold off on the documentation of the `U` key for now, since there's no corresponding entry in our main documentation page, and we'll need to decide on the language and placement of this line in that context first.

Signed-off-by: Jonathan Stone <[email protected]>
@jstone-lucasfilm jstone-lucasfilm changed the title MaterialX Viewer: Add a help UI Add keyboard shortcuts documentation to Viewer Sep 28, 2024
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Great work on this project, thanks @juandejoya!

@jstone-lucasfilm jstone-lucasfilm merged commit c3f4e5a into AcademySoftwareFoundation:main Sep 28, 2024
34 checks passed
@juandejoya juandejoya deleted the mtlx2011_viewer_add_help_ui branch October 2, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants