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

support custom widget entry labels #5705

Merged
merged 19 commits into from
Mar 17, 2023
Merged

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Feb 20, 2023

Pull Request Description

Implements #5640 and #5650

It made sense for me to implement those two together, as I wanted to make sure that the necessary widget API changes will support custom entry values for both dynamic and static data.

  • Added support for custom dropdown labels defined on the method annotations
  • Added shortening of static dropdown values, which resolves
dynamic dropdown - custom labels static dropdown - automatic shortening
image image

Important Notes

During implementation I had multiple data update order issues caused by FRP network forming a diamond shape. Two inputs that are often updated together were combined with all combinator, and that was further fed into the dropdown. This caused two updates to propagate through the whole network, and one of them was immediately outdated. To fix this and similar future scenarios, I've added an next_tick FRP node. It buffers the incoming events until the next browser microtask, preserving only the last received event. Currently if it is called inside a requestAnimationFrame callback, the effects of that processing will only be rendered in the next frame. Later this can be mitigated by delaying the rendering logic until the microtask queue is empty.

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 code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@Frizi Frizi force-pushed the wip/frizi/custom-dropdown-labels branch from ba22b7f to e1f01a8 Compare February 20, 2023 13:14
lib/rust/frp/src/microtasks.rs Outdated Show resolved Hide resolved
@wdanilo
Copy link
Member

wdanilo commented Mar 8, 2023

What is the status of this? CC @sylwiabr

@sylwiabr
Copy link
Member

@Frizi ?

@Frizi Frizi force-pushed the wip/frizi/custom-dropdown-labels branch 3 times, most recently from 26221a9 to 7a10ac9 Compare March 13, 2023 19:53
@Frizi Frizi marked this pull request as ready for review March 13, 2023 19:55
@Frizi
Copy link
Contributor Author

Frizi commented Mar 13, 2023

Since last change request

  • Finished microtick scheduler integration with frame loop, added detailed documentation around it.
  • Added import resolution for selected static widget entries. That works around buggy handling of fully qualified names in expressions.

CHANGELOG.md Outdated
@@ -111,6 +111,7 @@
- [Named arguments syntax is now recognized in IDE][5774]. Connections to
function arguments will now use named argument syntax instead of inserting
wildcards on all preceding arguments.
- [Added more user-friendly labels to dropdown widgets.][5705]
Copy link
Member

Choose a reason for hiding this comment

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

Please make it more wordy. Explanations in changeling should be understandable by no-code users. "more user-friendly" does not tell much .

Comment on lines +137 to +163
/// Delay and deduplicate the incoming events. Once an event is received, it will be emitted
/// after the current program execution finishes, but before returning to the event loop. If
/// multiple events are received within the same microtask, only the last one will be emitted.
///
/// Use [`Network::batch`] if you want to collect all emitted values within a microtask.
///
/// ```text
/// Input: ───────1──2─3───────────
/// Microtasks: ── ▶──── ▶──── ▶──── ▶──
/// Output: ─────────1─────3────────
/// ```
///
/// This node is useful when dealing with a diamond-shaped network, where a single source event
/// is flowing into multiple subgraphs, and those subgraphs eventually merge into a single node.
/// That final node would ordinarily receive multiple events for the same source event, once per
/// execution of each subgraph. It is usually a waste of resources to process those intermediate
/// events, and it can even lead to logical errors, as those events may contain partially
/// outdated data. This node can be used to deduplicate the events and only process the most
/// up-to-date one once all subgraphs have finished their execution.
///
/// Example network with a diamond-shaped subgraph:
/// ```text
/// clickPosition
/// / \
/// / \
/// posX absPosY
/// \ /
Copy link
Member

Choose a reason for hiding this comment

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

these docs rock!

@Frizi Frizi force-pushed the wip/frizi/custom-dropdown-labels branch from 3b1e2e1 to 829eeb3 Compare March 15, 2023 23:10
@Frizi Frizi force-pushed the wip/frizi/custom-dropdown-labels branch from 829eeb3 to 85af9d8 Compare March 15, 2023 23:31
@vitvakatu
Copy link
Contributor

I've added an next_tick FRP node.

Oh, that's nice. I think we can use it to fix CB in #5770 instead of creating a 0-ms timer. @farmaazon jfyi

@sylwiabr
Copy link
Member

@Frizi is it ready for QA?

@Frizi
Copy link
Contributor Author

Frizi commented Mar 16, 2023

@sylwiabr yes, it is waiting for QA now.

@wdanilo
Copy link
Member

wdanilo commented Mar 17, 2023

Alt Text

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

QA passed

@Frizi Frizi added the CI: Ready to merge This PR is eligible for automatic merge label Mar 17, 2023
@mergify mergify bot merged commit 9234d74 into develop Mar 17, 2023
@mergify mergify bot deleted the wip/frizi/custom-dropdown-labels branch March 17, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown display value shortening Single_Choice.values as Choice objects not Text
4 participants