android: Use LabelSync plugin #2281
Conversation
Explicitly setting the "use_labels" setting in the Android code might be antithetical to "plugins" but since there is no way to plug in or out modules in runtime in Android we might as well hardcode when needed. There are a couple of things to fix/investigate: * Currently the application has to be restarted when the settings have changed. * run_hook is not runned on main thread and a warning is printed. Is this a problem here? * Is a force upload/download option needed? * Should the 'android' part of the plugin, including the hooks, be more generically named? There isn't any Android specific things and other clients (iOS?) could use it...
Yes, as Calin said on Telegram, if the plugin is always present then the setting might as well always be present too. But I'm a bit confused about how the setting works. Why are there two names: use_labels and synchronize_labels? And I can't see anything that reads either of those settings, so how does it take effect?
I don't think that matters.
I'm not sure how this feature's supposed to be used: will it upload and download labels automatically, and if so when?
Rather than creating new hooks specifically for Android, I suggest you reuse the on_new_window and on_close_window hooks, since it looks like those are the de-facto open/close wallet hook names used by Qt plugins. Then following that pattern, you can create an Android "window" class with a single "wallet" member, and any Android-specific stuff can be added to that class in the future. But you might as well keep the "android.py" filename for the moment, since every other plugin file is named after its platform. If the iOS app uses it in the future, we can deal with it then. |
Ah, it should just be one and I fixed it in latest commits. And that setting is what enables the plugin. Any use_{plugin} boolean works as a switch, see
It will download when opening a wallet and upload when there is a change in any labels. There might be a sync problem when multiple instances of the same wallet is running with sync (for instance desktop and mobile) where it could be beneficial to explicitly resynchronize it.
Please see latest commit.
Ack! |
|
I'm late to the party here but..
Hmm. My memory is fuzzy (it's been a few months since I looked at how the "hooks" work) -- but I remember explicitly adding that warning because if hooks didn't run on the main thread something bad could happen -- since the "contract" is that the hooks may modify the UI in some cases. Not sure if that's relevant here. Also not really sure how the Andrioid app is architected -- but if it does end up touching the GUI it should be on the same thread as the GUI thread (typically this is the main thread in most GUI systems -- again I am an Android ignoramus so maybe that's not the case on Android? If that's the case then the run_hook warning needs to change for Android to account for that...). |
Ah, I see.
Yes, that'll have to be fixed, because there's no obvious way to restart an Android app process -- the user would have to to go into the settings and force-stop it. You could activate/deactivate the plugin using a settings observer like the one in the original commit.
Yes, Android has the same main thread rule. In this case the hooks don't actually touch the GUI, but the warning could be fixed by moving the hook calls from
OK, in that case, how about adding a label sync section to the settings, with the switch to enable the plugin, and two commands for force upload and download? The commands could use a TaskDialog subclass to run the network access on a background thread while keeping the UI alive. And the "dependency" attribute could be used to disable the commands if the plugin is disabled, as is currently done in the Fiat settings. |
|
The LabelSync plugin had been removed. |
Explicitly setting the "use_labels" setting in the Android code might be
antithetical to "plugins" but since there is no way to plug in or out
modules in runtime in Android we might as well hardcode when needed.
There are a couple of things to fix/investigate:
changed.
this a problem here?
generically named? There isn't any Android specific things and other
clients (iOS?) could use it...