-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Copy docs to getter in Properties macro #1490
Open
carlosmn
wants to merge
6
commits into
gtk-rs:main
Choose a base branch
from
carlosmn:cmn/properties-macro-docs
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
25f2800
examples: add some docs to the example using the Properties macro
carlosmn 97be519
glib-macros: Properties: copy property docs to getter
carlosmn 60f69d1
glib-macros: Properties: allow setting getter and setter docs
carlosmn bf86950
glib-macros: Properties: copy over the doc comments more elegantly
carlosmn 3581e84
glib-macros: extend `Properties` docs with the getter-setter docs
carlosmn f3799c2
glib-macros: add some generic comments for property notifications
carlosmn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would add Notify support as well while at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense yeah. Though I think it'll have to be in a different function because we call this one from within the loop inside
expand_impl_getset_properties
and the notify code is generated from a different one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added
# Notify
to copy the docs intonotify_$property
but now I'm not sure if that's what you mean or if it was supposed to be theconnect_$property_notify
as this one is maybe more likely to be something that a user of a class would need documentation about when that triggers or what the data would mean there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would auto-generate the connect_property_notify docs based on the notify one but not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often does one want to document the
notify_$property
method itself? Wouldn't that always just say to notify the property? I suppose you could mention when to call it as a user of the class, but I don't know how common that is for public types.But the
connect_$property_notify
method I think would benefit much more commonly from docs, no? You might want to specify non-obvious times it would get called, maybe values that depend on other values?I don't have experience with maintaining a public package, but looking at the docs for gtk itself, e.g. https://docs.gtk.org/gdk4/class.Surface.html (not that we have to match C but consistency is nice) the docs for the signals seem like the
connect_
side of the signals rather than the emitting side. Maybe it should be a# Signal
heading instead? That would also make it match tighter if we end up making it so there's some sort of list of signals in the docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I was conflating explicitly-defined signals and property change notifications, as the latter is not spelled out in the docs, so now I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because C doesn't generate a "safe" wrapper for signals compared to Rust bindings that is why there is no documentation for anything. Even less for the notify signal.
I would say, let us just skip having a special header for both Notify and the connect_notify and instead manually amend something like "Notify changes for property X" and "Listen to changes of property X".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've removed the handling for notify and added generic comments that just mention the notification name. We can always add some more "instructions" to the comment if this is something people want to do.