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

Consistency improvement with dock menu item #622

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

teamcons
Copy link

@teamcons teamcons commented Jan 1, 2025

Fixes #620

Ive tested both changing state in the dock and appmenu, for several apps, several times - it works.

Second commit removes usage of the "docked" variable, state would be tracked by the checkmenuitem state - more variables to keep in sync means more variables to keep track of and more opportunities for bugs down the line.
If you prefer keeping it, first commit works on its own.

i hope there arent too much new comments ? Needed to understand how its done and navigate. It may make the file new friendly for new contributors, but should have been its own separate commit.

@teamcons
Copy link
Author

teamcons commented Jan 1, 2025

While i was working to improve consistency i did a fourth commit, that, uh, could be discared if unwanted ?
A fix to #621

Current behaviour:
The add to dock entry is not always present, depending on whether we get an answer on dbus
appcenter/uninstall are always present, but if there is no answer on dbus, are insensitive

With this commit:
Here i replicated the behaviour for add to dock entry - it follows the same behaviour as the two other entries
just like appcenter we also check if the dock is present at all to build an entry. It was not the case before.

@stsdc stsdc requested a review from a team January 1, 2025 21:07
@danirabbit
Copy link
Member

Comments should usually be used to explain "why" if something is non-obvious. In this case things like construct is a part of GObject and is self-explanatory and always does the same thing in every object class so I'm not sure a comment that construct is the construct block really adds value.

Likewise something like "create menu item" right above new Gtk.CheckMenuItem this just describes exactly what the code already says with no further context so probably is not necessary

If you wanted to annotate the purpose of a class, that comment should be at the top of the class file. So for example explaining what a SwitcherooControl is, should be done in that class file and not every time that is instantiated elsewhere

See also: https://docs.vala.dev/developer-guides/documentation/valadoc-guide/01-00-quick-start.html

@teamcons
Copy link
Author

teamcons commented Jan 1, 2025

Comments should usually be used to explain "why" if something is non-obvious. In this case things like construct is a part of GObject and is self-explanatory and always does the same thing in every object class so I'm not sure a comment that construct is the construct block really adds value.

More of a visual help.

Should i do a commit to delete them all ? It doesnt take long.

@danirabbit
Copy link
Member

@teamcons I would remove comments that say the same thing that the code already says and leave in comments that explain why a thing is done or something that is not-obvious or counterintuitive

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.

Minor - Improve context menu label consistency
2 participants