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.

Sign up for GitHub

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

Jump to bottom

Change "Override Execution Context" button to "Record" button on nodes #9188

Merged
merged 28 commits into from
Mar 8, 2024
Merged

Change "Override Execution Context" button to "Record" button on nodes #9188

merged 28 commits into from
Mar 8, 2024

Conversation

somebody1234
Copy link
Contributor

@somebody1234 somebody1234 commented Feb 26, 2024 •
edited
Loading

Pull Request Description

  • Close Individually Output/Record enabled component enhancements #9164
    • Fix appearance of Record/Record Once icon in top menu
    • Change icon for overriding execution context to record icon
    • Unconditionally show per-node record icon if it is set
    • Remove the ability to override the execution context to disabled
    • Fix the icon for nodes with an overridden execution context always being the Enso icon

Important Notes

None

Screenshot

image

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.

@somebody1234
Copy link
Contributor Author

@farmaazon you mentioned you might have wanted to take a look at this

@somebody1234 somebody1234 changed the title Wip/sb/toggle recording on nodes Change "Override Execution Context" button to "Record" button on nodes Feb 26, 2024
@somebody1234 somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. x-new-feature Type: new feature request -gui labels Feb 26, 2024
@somebody1234 somebody1234 marked this pull request as ready for review February 28, 2024 17:33
@somebody1234
Copy link
Contributor Author

@AdRiley just a heads up: i've decided to remove support for the old behavior, since it doesn't really work well with the refactors.
i figure it should be simple enough to grab the old code from an old commit anyway

farmaazon
farmaazon approved these changes Feb 29, 2024
View reviewed changes
app/gui2/src/assets/icons.svg Show resolved Hide resolved
app/gui2/src/stores/graph/graphDatabase.ts Show resolved Hide resolved
@somebody1234 somebody1234 added the CI: Keep up to date Automatically update this PR to the latest develop. label Mar 6, 2024
@somebody1234 somebody1234 removed the CI: Keep up to date Automatically update this PR to the latest develop. label Mar 6, 2024
JaroslavTulach and others added 4 commits March 6, 2024 22:07
@JaroslavTulach @somebody1234
Use .. to identify autoscoped constructors (#9285)
5dc3765
@somebody1234
Extract tests to their own files (#9229)
5bb6c91 
Move tests to their own files, for consistency

None
@somebody1234
Fix tests
9101626
@farmaazon @somebody1234
Restore whenReady, as I do not see the freeze (#9262)
c1ad57b 
When cleaning up board, I stumbled upon #5851 I tried to replace whenReady, and it just worked. Perhaps the freeze was fixed during some electron version bump.

Tests on other platforms (I'm using Garuda Linux) advised.

Fixes #5851
@somebody1234
Copy link
Contributor Author

man, whatever happened here?

@somebody1234
Copy link
Contributor Author

oh i guess i messed up the merge...

@somebody1234 somebody1234 force-pushed the wip/sb/toggle-recording-on-nodes branch from f42f0d2 to 6bf8153 Compare March 7, 2024 10:37
@indiv0
Copy link
Contributor

indiv0 commented Mar 8, 2024

✅ QA passed

  • Fix appearance of Record/Record Once icon in top menu
  • Make the icons be consistent with the new icons for live/design mode in Replace execution context controls with record mode controls #9133
    • i.e. grey circle for off, red circle for on
  • Unconditionally show per-node record icon if it is set
  • Remove the ability to override the execution context to disabled
  • Fix the icon for nodes with an overridden execution context always being the Enso icon
image image image

indiv0
indiv0 approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@indiv0 indiv0 left a comment

Choose a reason for hiding this comment

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

LGTM

app/gui2/src/util/ast/node.ts
Comment on lines +15 to +17
export function initializePrefixes() {
prefixes = makePrefixes()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this must be called after initializeFFI we could encode that in the type system by making initializeFFI return a value that can't be constructed otherwise, and make that value an argument of this function.

But that might be overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i'm going to consider it as overkill for now. although it wouldn't be difficult to do it, it doesn't really match up with the rest of the codebase IMO.

Copy link
Contributor Author

@somebody1234 somebody1234 Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i mean, realistically in the actual app, both are called in the entrypoint in the correct order. so i don't think omitting it would be too big of a deal since there's only one place where they're called.

@mergify mergify bot merged commit 6c2b238 into develop Mar 8, 2024
32 of 34 checks passed
@mergify mergify bot deleted the wip/sb/toggle-recording-on-nodes branch March 8, 2024 04:31
@farmaazon farmaazon mentioned this pull request Mar 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdunkerley jdunkerley jdunkerley left review comments

@farmaazon farmaazon farmaazon approved these changes

@indiv0 indiv0 indiv0 approved these changes

@Frizi Frizi Awaiting requested review from Frizi Frizi is a code owner

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@vitvakatu vitvakatu Awaiting requested review from vitvakatu vitvakatu is a code owner

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

@radeusgd radeusgd Awaiting requested review from radeusgd

@GregoryTravis GregoryTravis Awaiting requested review from GregoryTravis

@AdRiley AdRiley Awaiting requested review from AdRiley AdRiley is a code owner

@PabloBuchu PabloBuchu Awaiting requested review from PabloBuchu PabloBuchu is a code owner

@MrFlashAccount MrFlashAccount Awaiting requested review from MrFlashAccount MrFlashAccount is a code owner

@4e6 4e6 Awaiting requested review from 4e6

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach

@hubertp hubertp Awaiting requested review from hubertp

@Akirathan Akirathan Awaiting requested review from Akirathan

Assignees
No one assigned
Labels
-gui CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge x-new-feature Type: new feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Individually Output/Record enabled component enhancements
6 participants
@somebody1234 @farmaazon @indiv0 @kazcw @jdunkerley @JaroslavTulach