Skip to content

Conversation

@rolandlo
Copy link
Contributor

This PR ports the Actions demo to Python

I have tried to stay as close as possible to the JS code. Not sure if I should have avoided using lambdas as in the Welcome demo, see comment. I used lambdas because here it's only for a single statement, whereas in the Welcome demo there were two statements.

The open_uri actions somehow don't work yet. Is this something that needs to be fixed on the "core" side or is there something wrong with my code?

@rolandlo rolandlo requested a review from sonnyp as a code owner October 27, 2023 06:32
@Hofer-Julian
Copy link
Contributor

I used lambdas because here it's only for a single statement

If it's only a single statement, it's fine to use lambdas IMO

@sonnyp sonnyp requested review from theCapypara and removed request for sonnyp October 27, 2023 10:02
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.

I'll leave it to @theCapypara to review his first Workbench PR :)

@rolandlo thanks! please consider adding yourself to the list of contributors in about.js

@sonnyp
Copy link
Contributor

sonnyp commented Oct 27, 2023

@theCapypara please do make sure to test the code, pasting the code directly into Workbench is the most efficient way for ports

@rolandlo rolandlo force-pushed the actions-demo-python branch from d579118 to a4e2878 Compare October 27, 2023 10:08
@rolandlo
Copy link
Contributor Author

rolandlo commented Oct 27, 2023

@rolandlo thanks! please consider adding yourself to the list of contributors in about.js

Done!

Copy link
Contributor

@theCapypara theCapypara left a comment

Choose a reason for hiding this comment

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

Thanks! In general this looks good and works perfectly, however could you make sure your code is formatted to follow PEP 8 and uses 4 spaces for indentation?

You can use pycodestyle to verify. You can ignore error E402 module level import not at top of file, since that isn't possible due to the require_version.

Also we may want to change the demo's blueprint file to also link to the PyGObject docs (or remove the GJS link), what do you think @sonnyp?


# Action with parameter
bookmarks_action = Gio.SimpleAction(
name = "open-bookmarks",
Copy link
Contributor

Choose a reason for hiding this comment

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

One example for PEP 8 is that there should be no spaces around the = in keyword arguments.

@rolandlo
Copy link
Contributor Author

rolandlo commented Oct 28, 2023

Thanks! In general this looks good and works perfectly, however could you make sure your code is formatted to follow PEP 8 and uses 4 spaces for indentation?

You can use pycodestyle to verify. You can ignore error E402 module level import not at top of file, since that isn't possible due to the require_version.

Done! It would be good to add these style requirements to the style guide, so everybody can learn about them without going through previous PR's.

Also we may want to change the demo's blueprint file to also link to the PyGObject docs (or remove the GJS link), what do you think @sonnyp?

I have added a commit adding that link. Let me know if I should remove the commit (or make changes to it).

Back to:

The open_uri actions somehow don't work yet. Is this something that needs to be fixed on the "core" side or is there something wrong with my code?

Is it only for me that the links from the bookmarks don't work (the links are not opened) with the Python code and the Actions Documentation section is disabled?
grafik

@theCapypara
Copy link
Contributor

theCapypara commented Oct 28, 2023

Oh, that's very odd, I somehow didn't even notice that the first time around.
Must be an issue in PyGObject, somehow? It's really odd since there's no code in either the JS/Python demo related to that, but I can't really imagine how the language bindings would break this.

grafik

However looking at the inspector it seems the "sensitive" property for the generated ModelButtons is somehow filled with the label...

@theCapypara
Copy link
Contributor

Oh wow, wait, disregard my previous comment even though I double and tripple checked before sending (and was very confused how this is even possible) I just was off by one line. That is the text.

@theCapypara
Copy link
Contributor

Okay this is not a bug in PyGObject, this is a general issue with the way the external previewers work at the moment: #715

@rolandlo
Copy link
Contributor Author

Should I squash all the commits (or at least the first two commits) into one?

@sonnyp sonnyp marked this pull request as draft October 29, 2023 10:48
@sonnyp
Copy link
Contributor

sonnyp commented Oct 29, 2023

Converting to draft until #715 is solved.

@sonnyp
Copy link
Contributor

sonnyp commented Oct 29, 2023

Should I squash all the commits (or at least the first two commits) into one?

No need, our default merging strategy is "Squash and merge"

@theCapypara
Copy link
Contributor

theCapypara commented Oct 29, 2023

IMO we could still merge this. There are already some other demos broken right now because of #715 / #110
And this is only a minor part of that demo.

@rolandlo rolandlo marked this pull request as ready for review October 29, 2023 11:24
@rolandlo
Copy link
Contributor Author

Ok then, let's squash and merge.

@rolandlo rolandlo merged commit 5306cf8 into workbenchdev:main Oct 29, 2023
@rolandlo rolandlo deleted the actions-demo-python branch October 29, 2023 11:36
@sonnyp
Copy link
Contributor

sonnyp commented Oct 29, 2023

@rolandlo please don't merge PRs. It is a task for maintainers.

Feel free to create branches though.

sonnyp added a commit that referenced this pull request Oct 29, 2023
sonnyp added a commit that referenced this pull request Oct 29, 2023
@rolandlo rolandlo restored the actions-demo-python branch October 29, 2023 11:53
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.

4 participants