Improve Synchronization UI #897

runkaiz · 2022-05-15T23:42:31Z

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.

Neui · 2022-05-16T11:06:32Z

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

gtg/GTG/gtk/browser/adaptive_button.py

Lines 1 to 8 in 5f72c8a

    
           import logging 
        
           import dataclasses 
        
           from typing import Optional, List, Tuple 
        
           from gi.repository import GObject, GLib, Gtk, Gdk 
        
           log = logging.getLogger(__name__)

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.

…ile directly from the description. Button hiding is now a one liner. Description for file storage updated to be more accurate since it is not a sync service. Remove extraneous print statement from previous commit.

The add panel now has the ability to detect which back-ends failed to initialize on start, and the developer can add corresponding error messages to help the user resolve dependency problems.

runkaiz · 2022-05-16T18:37:58Z

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

Neui

I am not sure if we should make a link to the XML file. At least for me, clicking on it does nothing, and if something did, it may encourage people to modify the file while GTG is still open.

Images would have been nice to look at. Just launch gnome-screenshot -i, select region, copy to clipboard and paste inside GitHub textbox thing.

After fixing the .append() issue:

I don't quite like how it looks like. Maybe allow Caldav to be selectable, but name it like "caldav (error)" or something, so if you have multiple backends you can see which ones have an error.

Neui · 2022-05-29T22:58:22Z

GTG/gtk/backends/configurepanel.py

@@ -151,7 +151,9 @@ def refresh_sync_status_label(self):
        Refreshes the Gtk.Label that shows the current state of this backend
        """
        if self.backend.is_default():
-            label = _("This is the default synchronization service")
+            xml_folder = GLib.filename_to_uri("/home/" + GLib.get_user_name() + "/.var/app/org.gnome.GTG/data/gtg/gtg_data.xml")


Don't hardcode paths like that, especially this is flatpak only and not everyone uses the flatpak version (such as distros, developers or users who test locally/want the latest version).

Seems self.backend.get_path() should work.

gtg/GTG/backends/backend_localfile.py

Line 100 in 5f72c8a

def get_path(self) -> str:

Neui · 2022-05-29T22:59:38Z

GTG/gtk/backends/configurepanel.py

-            label = _("This is the default synchronization service")
+            xml_folder = GLib.filename_to_uri("/home/" + GLib.get_user_name() + "/.var/app/org.gnome.GTG/data/gtg/gtg_data.xml")
+
+            label = "This is the default file storage backend. <a href='{}'>{}</a>.".format(xml_folder, _("Open the XML data file"))


Suggested change

label = "This is the default file storage backend. <a href='{}'>{}</a>.".format(xml_folder, _("Open the XML data file"))

label = _("This is the default file storage backend. <a href='{}'>Open the XML data file</a>.").format(xml_folder)

The outer text wasn't marked as translatable. Also you can inline the text for more context to the translators.

Neui · 2022-05-29T23:01:02Z

GTG/gtk/backends/addpanel.py

@@ -170,7 +172,16 @@ def on_combo_changed(self, widget=None):
        """
        backend_name = self.combo_types.get_selected()
        if backend_name is None:
+            if 'backend_caldav' in inactive_modules:
+                markup = '<big>Error: Python package \'caldev\' not installed.</big>'


Suggested change

markup = '<big>Error: Python package \'caldev\' not installed.</big>'

markup = '<big>{}</big>'.format(_('Error: Python package \'caldav\' not installed.'))

Typo in caldav, and make it translatable.

Neui · 2022-05-29T23:01:39Z

GTG/gtk/backends/addpanel.py

+                return
+
+
+            markup = '<big>Error: An unknown backend could not be loaded.</big>'


Suggested change

markup = '<big>Error: An unknown backend could not be loaded.</big>'

markup = '<big>{}</big>'.format(_('Error: An unknown backend could not be loaded.'))

Make it translatable.

Neui · 2022-05-29T23:11:47Z

GTG/backends/__init__.py

@@ -67,10 +67,12 @@ def __init__(self):
                # Something is wrong with this backend, skipping
                log.warning("Backend %s could not be loaded: %r",
                            module_name, exception)
+                inactive_modules.append(module_name)


Suggested change

inactive_modules.append(module_name)

inactive_modules.add(module_name)

Traceback (most recent call last): File "/app/lib/python3.9/site-packages/GTG/backends/__init__.py", line 65, in __init__ __import__(extended_module_name) File "/app/lib/python3.9/site-packages/GTG/backends/backend_caldav.py", line 30, in <module> import caldav ModuleNotFoundError: No module named 'caldav' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/app/lib/python3.9/site-packages/GTG/gtk/application.py", line 124, in do_startup for backend_dic in BackendFactory().get_saved_backends_list(): File "/app/lib/python3.9/site-packages/GTG/backends/__init__.py", line 70, in __init__ inactive_modules.append(module_name) AttributeError: 'set' object has no attribute 'append'

You need to use .add() since it is a set, not an list.

Neui · 2022-05-29T23:11:59Z

GTG/backends/__init__.py

                continue
            except Exception:
                # Other exception log as errors
                log.exception("Malformated backend %s:", module_name)
+                inactive_modules.append(module_name)


Suggested change

inactive_modules.append(module_name)

inactive_modules.add(module_name)

Ditto. You need to use .add() since it is a set, not an list.

Neui · 2022-05-29T23:14:00Z

GTG/backends/__init__.py

@@ -32,7 +32,7 @@
 from GTG.backends.generic_backend import GenericBackend

 log = logging.getLogger(__name__)
-
+inactive_modules = set()


Might be better to move this to the BackendFactory class below. The Borg class it inherits is basically making it a singleton (state shared on every instance).

nekohayo · 2024-02-26T18:37:43Z

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 · 2024-02-26T20:11:39Z

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

Hide button when default is detected

743914e

runkaiz changed the title ~~Hide button when default is detected~~ Improve Synchronization UI May 16, 2022

Improve Sync UI when missing packages

3e7dc1f

The add panel now has the ability to detect which back-ends failed to initialize on start, and the developer can add corresponding error messages to help the user resolve dependency problems.

runkaiz marked this pull request as ready for review May 16, 2022 18:36

Neui suggested changes May 29, 2022

View reviewed changes

diegogangl added the enhancement label Oct 8, 2022

nekohayo marked this pull request as draft February 26, 2024 18:37

nekohayo linked an issue Feb 26, 2024 that may be closed by this pull request

Synchronization backends dialog doesn't look nice when there are no truly available sync backends to add #837

Open

nekohayo added the plugins Plugins and extra backends label Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Synchronization UI #897

Improve Synchronization UI #897

runkaiz commented May 15, 2022

Neui commented May 16, 2022 •

edited

Loading

runkaiz commented May 16, 2022 •

edited

Loading

Neui left a comment

Neui May 29, 2022

Neui May 29, 2022

Neui May 29, 2022

Neui May 29, 2022

Neui May 29, 2022 •

edited

Loading

Neui May 29, 2022 •

edited

Loading

Neui May 29, 2022

nekohayo commented Feb 26, 2024

nekohayo commented Feb 26, 2024

	label = "This is the default file storage backend. <a href='{}'>{}</a>.".format(xml_folder, _("Open the XML data file"))
	label = _("This is the default file storage backend. <a href='{}'>Open the XML data file</a>.").format(xml_folder)

	markup = '<big>Error: Python package \'caldev\' not installed.</big>'
	markup = '<big>{}</big>'.format(_('Error: Python package \'caldav\' not installed.'))

		return


		markup = '<big>Error: An unknown backend could not be loaded.</big>'

	inactive_modules.append(module_name)
	inactive_modules.add(module_name)

Improve Synchronization UI #897

Are you sure you want to change the base?

Improve Synchronization UI #897

Conversation

runkaiz commented May 15, 2022

Neui commented May 16, 2022 • edited Loading

runkaiz commented May 16, 2022 • edited Loading

Neui left a comment

Choose a reason for hiding this comment

Neui May 29, 2022

Choose a reason for hiding this comment

Neui May 29, 2022

Choose a reason for hiding this comment

Neui May 29, 2022

Choose a reason for hiding this comment

Neui May 29, 2022

Choose a reason for hiding this comment

Neui May 29, 2022 • edited Loading

Choose a reason for hiding this comment

Neui May 29, 2022 • edited Loading

Choose a reason for hiding this comment

Neui May 29, 2022

Choose a reason for hiding this comment

nekohayo commented Feb 26, 2024

nekohayo commented Feb 26, 2024

Neui commented May 16, 2022 •

edited

Loading

runkaiz commented May 16, 2022 •

edited

Loading

Neui May 29, 2022 •

edited

Loading

Neui May 29, 2022 •

edited

Loading