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

Interaction resumption #9621

Merged
merged 12 commits into from
Apr 11, 2024
Merged

Interaction resumption #9621

merged 12 commits into from
Apr 11, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Apr 3, 2024

Pull Request Description

Prevent interactions such as an open dropdown from being disrupted by the temporary state of absent type information occurring after an edit.

Screen.Recording.2024-04-02.at.06.44.05.mov

All widget-edit interactions that are active when a component is to be unmounted save state and are suspended. When a new component defining a WidgetEditHandler is instantiated, if the component is found to be equivalent to a component that was suspended, and no other interaction has been initiated in the interim, the interaction is restarted using the suspended state.

Important Notes

  • Fix a bug caused by a variable tracking an interaction's active state getting out of sync with the interaction. WidgetEditHandler now provides a reactive active property; using it is simpler and avoids this type of bug in the future.
  • Fix a flickering bug that sometimes made it hard to open dropdowns by clicking the arrow.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw self-assigned this Apr 3, 2024
@kazcw kazcw linked an issue Apr 3, 2024 that may be closed by this pull request
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 3, 2024
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

I'd like to see a bit more docs for editHandler, as I don't quite grasp what is the relation between all those classes

Comment on lines 217 to 223
widgetId: WidgetId,
hooks: WidgetEditHooks,
parent: WidgetEditHandler | undefined,
portEditor: PortEditor,
widgetTree: {
currentEdit: WidgetEditHandler | undefined
} = injectWidgetTree(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that we use private readonly widgetId: WidgetId syntax in constructor parameters elsewhere, so it's both declared and set as property.

app/gui2/src/util/callTree.ts Outdated Show resolved Hide resolved
app/gui2/src/providers/widgetRegistry/editHandler.ts Outdated Show resolved Hide resolved
Comment on lines +246 to +248
pointerdown: (event, navigator) => {
if (!hooks.pointerdown) return false
return hooks.pointerdown?.(event, navigator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where the propagation is done?

readonly active: Ref<boolean>
private readonly interactionHandler
private readonly resumable: PortEditResumeData
private readonly interactions = new Array<PortEditSubinteraction>()
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a difference between PortEditInteraction and PortEditSubinteraction?

interface Endable {
end?(origin: WidgetId): void
}
/** A child interaction of a @{link PortEditInteraction} */
Copy link
Contributor

Choose a reason for hiding this comment

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

What is "child interaction"? Is it the same as "child's interaction"? In this page docs, the only parent/child relation is this of Widgets.

Comment on lines 139 to 140
* Obtains a top-level interaction to edit a port (see {@link PortEditInteraction}), which may be a pre-existing
* ongoing interaction, or a newly-started interaction. */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name does not quite reflect the usage.

@kazcw
Copy link
Contributor Author

kazcw commented Apr 10, 2024

I rearranged the file to introduce concepts in a more logical order, and added docs, especially to WidgetEditInteraction, which is the most important class besides WidgetEditHandler and its public interface.

@kazcw kazcw requested a review from farmaazon April 10, 2024 13:37
parent: WidgetEditHandler | undefined,
portEditor: PortEditor,
widgetTree: {
private readonly widgetId: WidgetId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this, the initializations (lines 57 onwards) are redundant.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2024
@mergify mergify bot merged commit b631745 into develop Apr 11, 2024
37 checks passed
@mergify mergify bot deleted the wip/kw/interaction-resumption branch April 11, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add multi-selection support to dropdown widget
2 participants