gtg icon indicating copy to clipboard operation
gtg copied to clipboard

Improve Synchronization UI

Open runkaiz opened this issue 2 years ago • 4 comments

Partially implements #837.

This is my first pull-request (well, first time attempting to contribute to a public open source project in fact). Code changes are minimum so please give me some advice if I am doing something wrong here, any help would be appreciated, just trying to gain some experience.

runkaiz avatar May 15 '22 23:05 runkaiz

Not entirely sure if you want some feedback now, but:

  • You hide the button, but never make it .show() it again (at least from what I can see from the changes, I haven't tested the change). There is .set_visible() you can use with an boolean parameter. if you want to make it a one-liner.
  • print() is OK when debugging, just make sure to remove them before committing/submitting. (I'd say in a draft PR it would be OK, since code is likely to be changed anyway). You could also use the logging system to print stuff out like log.debug("xyz") if you think it could be useful later when debugging (you need to import logging and log = logging.getLogger(__name__) near the top of the file, like for example https://github.com/getting-things-gnome/gtg/blob/5f72c8a9319e06d42967fb8461ea832e8cd095d9/GTG/gtk/browser/adaptive_button.py#L1-L8

For the link stuff (mentioned in the linked issue), https://docs.gtk.org/gtk3/class.Label.html#links would be a start.

Edit: Also seeing it now, consider creating a branch in your repository to do the changes rather in your master. So you can name it better and don't accidentally overwrite/combine changes if you want to submit multiple changes (in separate PRs). I don't think GitHub lets you change the branch to merge from, so you can keep it for now.

Neui avatar May 16 '22 11:05 Neui

I believe most if not all of the requested changes have been applied. I might have misinterpreted the part about hiding the enable / disable button on the default back-end so I would appreciate it if someone can help me review this.

edit: typo

runkaiz avatar May 16 '22 18:05 runkaiz

The big refactoring has finally landed (see the pinned 0.7 tracking issue for details)!

Please rebase this to the latest master. Apologies for the inconvenience; the new codebase is expected to be much better to work with.

nekohayo avatar Feb 26 '24 18:02 nekohayo

Once this gets rebased, I would like @jaesivsm to help reviewing this.

nekohayo avatar Feb 26 '24 20:02 nekohayo