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

library: Add location entry #355

Merged
merged 2 commits into from Jul 6, 2023
Merged

library: Add location entry #355

merged 2 commits into from Jul 6, 2023

Conversation

SoNiC-HeRE
Copy link
Contributor

Closes #350

Implemented the following changes:

  • Made the UI interactive by using Revealer
  • Used Gtk.SpinButton and Gtk.Dropdown for changing values
  • Covered all 8 properties

Looking for an early review

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Looks good!

image

Could you make the stop button sensitive: false when the session is started and vice versa? Like ASHPD.

Something I noticed is that pressing "Run" after starting a session breaks the demo, could you look into that?

Comment on lines 31 to 36
adjustment: Adjustment {
lower: 0;
upper: 100;
step-increment: 1;
value: 0;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

only 2 spaces indent please, example:

Suggested change
adjustment: Adjustment {
lower: 0;
upper: 100;
step-increment: 1;
value: 0;
};
adjustment: Adjustment {
lower: 0;
..
};

}
}

Adw.ActionRow {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use a Adw ComboRow instead of an ActionRow + DropDown

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 23, 2023

Looks good!

image

Could you make the stop button sensitive: false when the session is started and vice versa? Like ASHPD.

Something I noticed is that pressing "Run" after starting a session breaks the demo, could you look into that?

I've applied all the mentioned changes and it seems to work fine and does'nt break even when a session has started and i press run button again; maybe can you try again and lemme know if that happens again?

@SoNiC-HeRE SoNiC-HeRE requested a review from sonnyp June 23, 2023 10:50
@sonnyp
Copy link
Contributor

sonnyp commented Jun 23, 2023

It happens when the session isn't closed before pressing "Run" again.

I'm not sure what we can do about it at the moment. Possibly we have to wait until we support opening JS demo in a separate process.

I'll investigate.

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Let's merge this, I have filed an issue about the problem which we can tackle later

https://github.com/sonnyp/Workbench/issues/388

@sonnyp sonnyp merged commit c44d4a4 into main Jul 6, 2023
@sonnyp sonnyp deleted the sonic/location branch July 6, 2023 13:49
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Aug 13, 2023
sonnyp pushed a commit that referenced this pull request Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libportal location
2 participants