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

envvars-dialog: Allow to paste var=value and fix validation #3679

Merged
merged 8 commits into from
Jan 13, 2025

Conversation

jntesteves
Copy link
Contributor

@jntesteves jntesteves commented Jan 7, 2025

  • Rework the envvars dialog to allow copy/pasting complete variable assignments instead of having to input first the name and later the value.
  • Fix the input validation which had some inverted logic and make it behave as expected.
  • Make the panel leaner by removing unnecessary headers and periods at the end of labels.
  • Port to AdwDialog

image

* Rework the envvars dialog to allow copy/pasting complete variable assignments instead of having to input first the name and later the value.
* Fix the input validation which had some inverted logic and make it behave as expected.
* Make the panel leaner by removing unnecessary headers and periods at the end of labels.
@jntesteves jntesteves force-pushed the sane-envvars-dialog branch from fae1add to 35c77c5 Compare January 7, 2025 00:42
Traverse AdwEntryRow's widget tree to remove visibility of the title labels and change alignment of the text entry to center.

This implementation is tightly coupled with the internal representation of AdwEntryRow. Checks were added to prevent crashing if that representation changes, but in such case the layout may get askew.
@jntesteves jntesteves force-pushed the sane-envvars-dialog branch from 8b17cfc to a711a4f Compare January 8, 2025 02:50
@jntesteves jntesteves marked this pull request as ready for review January 8, 2025 02:57
bottles/frontend/ui/dialog-env-vars.blp Outdated Show resolved Hide resolved
bottles/frontend/windows/envvars.py Outdated Show resolved Hide resolved
bottles/frontend/windows/envvars.py Outdated Show resolved Hide resolved
bottles/frontend/ui/env-var-entry.blp Outdated Show resolved Hide resolved
@jntesteves jntesteves force-pushed the sane-envvars-dialog branch 2 times, most recently from 704fc2b to 56f959d Compare January 9, 2025 03:19
Copy link
Member

@TheEvilSkeleton TheEvilSkeleton left a comment

Choose a reason for hiding this comment

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

Some things that should be fixed:

  • Inserting a character and removing it will keep showing the check button:
    image
    Instead, the check button shouldn't appear.
  • The moment you type something, the row turns red, and the check appears:
    image
    Instead, the row should keep the neutral color without displaying the check. The error should only appear when the user explicitly adds the equal sign (=) and removes it.
  • If you remove the last defined environment variable, the "No environment variables defined" label doesn't appear, when it should.
  • It's pretty easy to accidentally close the dialog without saving your changes, so there could be an AlertDialog to confirm the user intends to discard.
  • I think the dialog is too big for what it is, perhaps you could adjust the height to content-height: 550; and content-width: 500;?

* Only show the apply button when the value is valid
* Don't add error style to the text field when the value is empty or when it doesn't contain an equal sign
@jntesteves
Copy link
Contributor Author

* Inserting a character and removing it will keep showing the check button:
  Instead, the check button shouldn't appear.

* The moment you type something, the row turns red, and the check appears:
  Instead, the row should keep the neutral color without displaying the check. The error should only appear when the user explicitly adds the equal sign (`=`) and removes it.

Done. A tiny bit hackey because of the way the apply button works. The widget doesn't have a method to just hide it without outright disabling the feature. So I disable+enable it just to reset its state when invalid. Works beautifully :chefs_kiss:

* If you remove the last defined environment variable, the "No environment variables defined" label doesn't appear, when it should.

Done.

* It's pretty easy to accidentally close the dialog without saving your changes, so there could be an [AlertDialog](https://gnome.pages.gitlab.gnome.org/libadwaita/doc/main/class.AlertDialog.html) to confirm the user intends to discard.

This is not a regression but an issue this dialog already had, so I think it can be deferred to another PR as to not block and delay the improvements in this one.

* I think the dialog is too big for what it is, perhaps you could adjust the height to `content-height: 550;` and `content-width: 500;`?

Now that the variable names are on the same line, some values on longer variables don't fit the screen. So I increased the width from the previous 500 to 600 to give more space for the names. That width is still a bit smaller than the width of the rows on any settings screen, which I think is a fair width for properties otherwise it wouldn't be the default on all the Adw properties widgets. The dialog is responsive so this is the max width, it scales down when space is limited.

As for the height, we have an issue that AdwDialog is not resizeable, so dialogs that will potentially contain long lists have to be tall. Again, they are responsive and scale down on small screens.

@TheEvilSkeleton
Copy link
Member

Good point on every disagreement. I'll take a look at the code

bottles/frontend/utils/gtk.py Outdated Show resolved Hide resolved
bottles/frontend/utils/gtk.py Outdated Show resolved Hide resolved
bottles/frontend/utils/gtk.py Outdated Show resolved Hide resolved
bottles/frontend/windows/envvars.py Show resolved Hide resolved
@TheEvilSkeleton TheEvilSkeleton mentioned this pull request Jan 12, 2025
* Move non-obvious code blocks into methods to better describe why it is needed
* Update all comments
Copy link
Member

@TheEvilSkeleton TheEvilSkeleton left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. The only thing that needs to be done is updating the POTFILES file. After that, you're free to merge.

@jntesteves
Copy link
Contributor Author

Everything looks good to me. The only thing that needs to be done is updating the POTFILES file. After that, you're free to merge.

Is this what you mean? I've regenerated the translation files. It seems that hasn't been done in a while, it made hundreds of changes coming from many changed files.

@TheEvilSkeleton
Copy link
Member

My apologies, I thought you changed the file names but my eyes deceived me. Feel free to revert it 😅

@TheEvilSkeleton
Copy link
Member

Everything looks good to me. Merging :)

@TheEvilSkeleton TheEvilSkeleton merged commit b2293af into bottlesdevs:main Jan 13, 2025
4 checks passed
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