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

extend AboutWindow #157

Merged
merged 5 commits into from
Mar 4, 2024
Merged

Conversation

nylki
Copy link
Contributor

@nylki nylki commented Mar 1, 2024

This PR implement previously missing bindings for the following fields in AboutWindow (Adwaita):

  • applicationIcon
  • releaseNotes
  • comments
  • debugInfo
  • developers
  • designers
  • artists
  • documenters
  • licenseType
  • legalSections
  • credits
  • acknowledgements
  • links

I also added an example for the Adwaita AboutWindow with a screenshot and everything. 🙂
Originally, I planned to implement all the missing fields.
But since I lack experience with both nim and GTK development, I didn't feel comfortable touching the translators field because it appears to ties into app localization which I was not sure if it was even supported in owlkettle yet.

Some other things I wasn't sure about when implementing the bindings:

  • legalSections: I defined a custom type to be easier able to fill the list. However I am not sure if thats the way to go here, or if it could be made more concise with standard dictionary/tuple types (which I didn't manage to do).

  • optional section name in credits: I didn't make it optional in this MR, as I wasn't sure what's the best way to deal with optional values is in nim/owlkettle (see above).

+ implement previously missing bindings for the following fields:

- applicationIcon
- releaseNotes
- comments
- debugInfo
- developers
- designers
- artists
- documenters
- licenseType
- legalSections
- credits
- acknowledgements
- links

+ adds more examples and extends the documentation
docs/widgets.md Show resolved Hide resolved
by adding "about_window.nim" to uncompileable examples.
@nylki nylki changed the title feat(adw): extend AboutWindow extend AboutWindow Mar 1, 2024
Copy link
Owner

@can-lehmann can-lehmann left a comment

Choose a reason for hiding this comment

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

Looks pretty good already 😄 I played with the example a bit and it works quite well. Here are some small comments.

Comment on lines 814 to 832
LICENSE_UNKNOWN = 0
LICENSE_CUSTOM = 1
LICENSE_GPL_2_0 = 2
LICENSE_GPL_3_0 = 3
LICENSE_LGPL_2_1 = 4
LICENSE_LGPL_3_0 = 5
LICENSE_BSD = 6
LICENSE_MIT_X11 = 7
LICENSE_ARTISTIC = 8
LICENSE_GPL_2_0_ONLY = 9
LICENSE_GPL_3_0_ONLY = 10
LICENSE_LGPL_2_1_ONLY = 11
LICENSE_LGPL_3_0_ONLY = 12
LICENSE_AGPL_3_0 = 13
LICENSE_AGPL_3_0_ONLY = 14
LICENSE_BSD_3 = 15
LICENSE_APACHE_2_0 = 16
LICENSE_MPL_2_0 = 17
LICENSE_0BSD = 18
Copy link
Owner

@can-lehmann can-lehmann Mar 2, 2024

Choose a reason for hiding this comment

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

Style: Enum entries of the public enum should be PascalCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted them to PascalCase, however I am not sure how well it works for licenses like LicenseLGPL2_1_Only 🤔.
let me know if they are fine now or if can think of a better way for the ones containing both appreviations and decimal numbers.

docs/widgets.md Show resolved Hide resolved
examples/widgets/adw/about_window.nim Outdated Show resolved Hide resolved
Comment on lines 852 to 854
license: string ## Sets the license as a custom text if it can’t be set via licenseType (when set licenseType will be set to GTK_LICENSE_CUSTOM).
licenseType: LicenseType ## Sets the license for from a list of known license.
legalSections: seq[LegalSection] ## Adds an extra section to the Legal page. Extra sections will be displayed below the application’s own information. The parameters copyright, licenseType and license will be used to present the it the same way as AboutWindow:copyright, AboutWindow:licenseType and AboutWindow:license are for the application’s own information. See those properties for more details. This can be useful to attribute the application dependencies or data.
Copy link
Owner

Choose a reason for hiding this comment

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

Using the GTK doc comments is an issue as

  1. They often do not apply to owlkettle. E.g. in this case copyright, licenseType and license are not parameters to legalSections, as it is a field, not a function.
  2. GTK is LGPL licensed. I am not a lawyer, but from my understanding, as this license also applies to the (doc) comments, using GTK's doc comments would require owlkettle to be relicensed under LGPL (or a compatible license).

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 adjusted the comments to not be a direct copy of the GTK docs and to better fit owlkettle usage in some cases.

examples/widgets/adw/about_window.nim Outdated Show resolved Hide resolved
result = gui:
Window:
title = "About Dialog Example"
defaultSize = (400, 200)
Copy link
Owner

Choose a reason for hiding this comment

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

I would consider increasing the window size a bit, as the small size seems to cause weird behavior when attempting to move the about 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 did increase the size as you requested, but did not observe any weird behaviour myself (Fedora 39, libadwaita 1.4.3-2).

Comment on lines 863 to 865
credits: seq[(string, seq[string])] ## Adds additional credit sections with customizable titles
acknowledgements: seq[(string, seq[string])] ## Adds acknowledgment sections with customizable titles
links: seq[(string, string)] ## Adds additionals links placed in the details section
Copy link
Owner

Choose a reason for hiding this comment

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

I would use tuples with named fields (tuple[x: float]) instead of anonymous tuples. Otherwise the purpose of the fields is not immediately obvious.

@can-lehmann
Copy link
Owner

can-lehmann commented Mar 2, 2024

Regarding the legalSections field: Using a custom type a completely valid approach imo.

Regarding optional values: I would say that if possible, use the empty string value as the NULL value. If the widget has different behavior when passing an empty string vs. passing NULL, use std/options.

nylki and others added 2 commits March 3, 2024 10:45
Co-authored-by: Can Lehmann <85876381+can-lehmann@users.noreply.github.com>
- use PascalCase for LicenseType enum
- use tuples with named fields instead
- tweak doc comments
- increase example window size
@nylki nylki force-pushed the extend-adwaita-aboutwindow branch from 99ec4f9 to d384c50 Compare March 3, 2024 10:37
@nylki
Copy link
Contributor Author

nylki commented Mar 3, 2024

I appreciate you taking the time to look into the PR! :)

Regarding optional values: I would say that if possible, use the empty string value as the NULL value. If the widget has different behavior when passing an empty string vs. passing NULL, use std/options.

Thanks! As it turns it in the case of the credits section ( adw_about_window_add_credit_section), using an empty string ("") for the title, already works as intended.

@can-lehmann can-lehmann merged commit a678f9b into can-lehmann:main Mar 4, 2024
3 checks passed
@can-lehmann
Copy link
Owner

can-lehmann commented Mar 4, 2024

Looks good, thanks for contributing!

I fixed some typos before merging. I also changed the example values for the license field from LicenseArtistic to LicenseMIT_X11, as I am not familiar with the Artistic license and was not sure if it is compatible with MIT (Owlkettle) / LGPL (GTK) (it might very well be, I just don't know). So just to be sure and not accidentally imply a compatibility to potential owlkettle users, I changed it to LicenseMIT_X11.

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