Skip to content

Conversation

@Diego-Ivan
Copy link
Contributor

While Workbench indicates which programming languages are not available for an individual demo there is no way to know it before opening it, and given that it now supports three (and probably some more in the future), I thought it would be a nice addition if this was indicated in the Demo Library window per demo, and I came up with this:

Captura desde 2023-10-08 19-58-56

If a programming language is available its icon is added to the right of the row. This is done by checking if the default source files (main.vala and code.rs) exist within the demo directory.

I used the same symbolic icons that are used in GNOME Builder for the files' mime types, although I'm not very sure about the JS icon as it not as obvious as the other two, and probably should be replaced by another.

Anyways, if there is any improvements/changes I can make to this PR, I will implement them as soon as possible 😄

This adds a key to the the Rust and Vala languages: `default_file`,
which contains their expected source file name.

It also adds a function, `demoSupportsLanguage`, which checks if the
`default_file` for the language exists in the demo directory.
@Diego-Ivan Diego-Ivan requested a review from sonnyp as a code owner October 9, 2023 05:16
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.

Thanks, this is a great idea.

@bragefuglseth suggested a redesign of the library in #510 which includes labels for supported languages.

I don't like the icons too much, they don't look great, and it might be unclear what they are.

What about showing the languages as labels on a new line below the description? Similar to #510

@Diego-Ivan
Copy link
Contributor Author

Thanks for the feedback. It looks nicer and it's definitely clearer with the labels instead of the icons 😄

imagen

@sonnyp sonnyp changed the title feature (Library): Show which languages are available in each demo library: Show which languages are available in each entry Oct 11, 2023
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 great thanks!

I fixed the linter errors and merged main, make sure to git pull. To avoid linter errors in the future please see https://github.com/sonnyp/Workbench/blob/main/CONTRIBUTING.md#setup so that the pre-commit hook takes care of formatting for you.

When and tried and since the tag has a hover state and behaves like a button I expected the demo to open with the language I clicked on.

I think that would make a great addition, could you add that in? If not, please don't make the tag behave like a button.

Comment on lines +192 to +196
export function demoSupportsLanguage(demo, id) {
const language = getLanguage(id);
const demo_dir = demos_dir.get_child(demo.name);
return demo_dir.get_child(language.default_file).query_exists(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For each demo (> 100) this checks twice if a file exist
that's > 200 blocking fs operations

It's not ideal. The Library should already open faster.
I've wanted to optimize it anyway so I can take care of it later 👍

What I have in mind is to build a Library index at build time so there is no need to query file system to build the Library window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also worried about all the I/O, and although it did not seem to slow it down that much, with a hundred more demos it will likely be a problem.

I thought about adding the languages available in each .json file but it is something that can be easily forgotten when adding/porting a demo. Doing it in build time is definitely better.

#createLanguageTag(language_name) {
return new Gtk.Label({
label: language_name,
css_name: "button",
Copy link
Contributor

@sonnyp sonnyp Oct 11, 2023

Choose a reason for hiding this comment

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

This should be either a label or a button. Don't overwrite the css_name.

@Diego-Ivan
Copy link
Contributor Author

I fixed the linter errors and merged main, make sure to git pull. To avoid linter errors in the future please see https://github.com/sonnyp/Workbench/blob/main/CONTRIBUTING.md#setup so that the pre-commit hook takes care of formatting for you.

Great! Thanks

When and tried and since the tag has a hover state and behaves like a button I expected the demo to open with the language I clicked on.

I think that would make a great addition, could you add that in? If not, please don't make the tag behave like a button.

Sounds good, I will implement it soon too.

Each language tag is now a button, which when clicked open the
demo in said language.
Specify where the styling for the language tags comes from
@Diego-Ivan
Copy link
Contributor Author

I've implemented the feature to open the demo in the language the user clicks on, it's a neat addition! Also, I added a link to Builder's stylesheet where I took the styling the language tags from. I'm not sure if it is possible to reuse other classes from Libadwaita other than .pill for them, so I will leave it as is.

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.

Great work, thanks a lot.

I made minor changes, I hope you don't mind. None of them is interesting or important and I didn't want to block this great feature.

Let me know if you'd rather me not do that in the future.

eca12d6

0f55da3

break;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

well done 👍

Some demo don't have code.
For Vala / Rust it works fine because we have a placeholder.

For JavaScript there is nothing in that case.
So it says reports as available with Rust/Vala but not JavaScript.

Most likely we would want to remove all tags for demos that don't have code but that's a bit more work.
Maybe later.
@sonnyp sonnyp merged commit f403050 into workbenchdev:main Oct 14, 2023
@sonnyp
Copy link
Contributor

sonnyp commented Oct 14, 2023

@sonnyp
Copy link
Contributor

sonnyp commented Oct 14, 2023

Let me know if you have a Mastodon account.

@Diego-Ivan
Copy link
Contributor Author

I made minor changes, I hope you don't mind. None of them is interesting or important and I didn't want to block this great feature.

No worries. Thanks for reviewing this :)

Let me know if you have a Mastodon account.

I do! It's dimmednerd@mastodon.social 😄

@sonnyp sonnyp mentioned this pull request Jul 2, 2024
10 tasks
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.

2 participants