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

app/proc: Load real procedures and upgrade viz #58

Merged
merged 8 commits into from
Oct 6, 2020
Merged

Conversation

jacobdeery
Copy link
Contributor

@jacobdeery jacobdeery commented Sep 9, 2020

  • Add a menu for opening a ProcLang file in the application (available
    with Ctrl-O)
  • Add utility functions to the ProceduresEngine for loading a new suite
    and resetting
  • Add visualization of ProcedureStep conditions/transitions
  • Add a bunch of TODOs

The procedures pane will look like this for now:
image

We haven't merged #54 yet, so there's no pretty string representation, but once we get those in it will look a bit more concise.


This change is Reviewable

Base automatically changed from proclang to master September 15, 2020 00:24
wendi-yu
wendi-yu previously approved these changes Sep 16, 2020
Copy link
Member

@wendi-yu wendi-yu left a comment

Choose a reason for hiding this comment

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

Yeah looks good! I tried out loading the example proclang doc from the menu and moving through the steps, everything seems to work well (including the highlighting we talked about :0) Admittedly I don't have a ton of insight to offer on the Qt stuff though :)

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @CODE-TITAN, @jacobdeery, and @wendi-yu)


application/procedures_bridge.py, line 156 at r1 (raw file):

    @Slot()
    def stop(self):
        # TODO(jacob): Should this reset the plumbing engine as well?

I think it makes sense for it to reset the plumbing engine - if we didn't, we'd end up with the procedures reset and ready to work on a plumbing engine in a modified state. If we just wanted to do something custom to the current plumbing engine state I think it makes more sense to pause than to stop?


application/resources/ProceduresPane.qml, line 73 at r1 (raw file):

            width: proceduresList.width
            height: procedureColumn.height + 10
            color: ListView.isCurrentItem ? "#d8fff7" : (index % 2 == 0 ? "#f2f2f2" : "#f9f9f9")

Is there any way to name the colours more friendly-ily? It's a minor inconvenience since I'm pretty sure most editors give a sample colour anyways but just asking - maybe worth looking into if there's a way to globally assign names to theme colours or something, once UI becomes a thing that we care about more.

Copy link
Contributor Author

@jacobdeery jacobdeery left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @CODE-TITAN and @wendi-yu)


application/procedures_bridge.py, line 156 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

I think it makes sense for it to reset the plumbing engine - if we didn't, we'd end up with the procedures reset and ready to work on a plumbing engine in a modified state. If we just wanted to do something custom to the current plumbing engine state I think it makes more sense to pause than to stop?

Yeah, I would tend to agree with that. I'll leave that for a future PR because right now there isn't a plumbing engine reset() method.


application/resources/ProceduresPane.qml, line 73 at r1 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Is there any way to name the colours more friendly-ily? It's a minor inconvenience since I'm pretty sure most editors give a sample colour anyways but just asking - maybe worth looking into if there's a way to globally assign names to theme colours or something, once UI becomes a thing that we care about more.

I've turned these into properties instead of hardcoded inline strings. Pulling them out into a global theme will probably require a bit more work; I'll open an issue and leave that for a future PR.

@jacobdeery jacobdeery requested a review from wendi-yu September 19, 2020 23:53
@jacobdeery jacobdeery added application Affects the Topside application procedures Affects procedures or ProcLang code labels Sep 20, 2020
wendi-yu
wendi-yu previously approved these changes Sep 24, 2020
Copy link
Member

@wendi-yu wendi-yu left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @CODE-TITAN and @wendi-yu)


application/procedures_bridge.py, line 156 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

Yeah, I would tend to agree with that. I'll leave that for a future PR because right now there isn't a plumbing engine reset() method.

Huh, I guess there isn't. Sounds like it could be a good first issue :0


application/resources/ProceduresPane.qml, line 73 at r1 (raw file):

Previously, jacobdeery (Jacob Deery) wrote…

I've turned these into properties instead of hardcoded inline strings. Pulling them out into a global theme will probably require a bit more work; I'll open an issue and leave that for a future PR.

Cool cool, I was just thinking about how tailwind does it (global JSON config file, iirc). Could be a cool new thing to implement later on, imo it definitely makes UI stuff cleaner and nicer to work with - but it'll definitely be more helpful if/when we have a decided UI with actual theme colours to implement and stuff :)

- Add a menu for opening a ProcLang file in the application (available
with Ctrl-O)
- Add utility functions to the ProceduresEngine for loading a new suite
and resetting
- Add visualization of ProcedureStep conditions/transitions
- Add a bunch of TODOs
@jacobdeery jacobdeery requested a review from wendi-yu October 3, 2020 19:47
@jacobdeery
Copy link
Contributor Author

Ready to go now that all of the other prerequisite PRs are in!

Copy link
Member

@wendi-yu wendi-yu left a comment

Choose a reason for hiding this comment

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

Ah very cool I like it! I assume the newly rebased stuff was code reviewed. I can't step through the procedure loaded from example.proc when I run the application, not sure if that's expected? Looks good otherwise. It'll be really exciting once we connect this to loading in a plumbing engine system from PDL as well.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @CODE-TITAN, @jacobdeery, and @wendi-yu)


application/procedures_bridge.py, line 136 at r3 (raw file):

    @Slot()
    def loadProcedureSuite(self):
        # TODO(jacob): Make this menu remember the last file opened

Good first issue?

Copy link
Contributor Author

@jacobdeery jacobdeery left a comment

Choose a reason for hiding this comment

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

I can't step through the procedure loaded from example.proc when I run the application, not sure if that's expected?

Discussed offline, expected behaviour for the procedure in question

It'll be really exciting once we connect this to loading in a plumbing engine system from PDL as well.

Stay tuned for #79!

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @CODE-TITAN and @wendi-yu)


application/procedures_bridge.py, line 136 at r3 (raw file):

Previously, wendi-yu (Wendi Yu) wrote…

Good first issue?

Wilco

@jacobdeery jacobdeery merged commit d60d7c5 into master Oct 6, 2020
@jacobdeery jacobdeery deleted the proc_opener branch October 6, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application Affects the Topside application procedures Affects procedures or ProcLang code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants