Skip to content

Fix syncthing plugin #223

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ngencokamin
Copy link

Syncthing wasn't working properly. Was throwing errors about st not being defined any time the user types in albert, and the error [crit:albert.python] Unable to cast Python instance of type <class 'NoneType'> to C++ type 'QString' when opening settings, displaying a blank window instead of the api key field. This PR:

  • Adds handling to make sure the api key is never None type, fixing the settings issue
  • Adds handling for missing and invalid api keys
    • Shows distinct messages for each scenarios

Screenshots
image
Valid api key

image
Invalid api key

image
No api key

@ngencokamin
Copy link
Author

I reverted my changes on lines 58-64. I'd changed them while experimenting with why settings was broken and forgot to revert. As an aside, as far as I can tell the option does nothing, but I added it back just to avoid changing anything I didn't need to.

@ngencokamin
Copy link
Author

Oh forgot to include, here's a before and after of the settings menu
image
image

@ManuelSchneid3r
Copy link
Member

@ngencokamin lets continue the discussion here. what have you changed to make the config widget work?

@ngencokamin
Copy link
Author

@ngencokamin lets continue the discussion here. what have you changed to make the config widget work?

Hey sorry for the slow reply, been crazy busy with work. Lemme run through and break down my changes and why I made them/what they do

Copy link
Author

@ngencokamin ngencokamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a lil breakdown of the code choices I made! Sorry it took so long 😭

@@ -22,14 +22,16 @@

class Plugin(PluginInstance, GlobalQueryHandler):

config_key = 'syncthing_api_key'
config_key = 'api_key'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to make the config key consistent with references throughout the rest of the script

Comment on lines +33 to +34
if self._api_key is None:
self._api_key = ''
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the widget not appearing with error 12:26:44 [crit:albert.python] Unable to cast Python instance of type <class 'NoneType'> to C++ type 'QString'

Basically just initializes api_key to an empty string if an api key isn't found in the config. Otherwise it breaks because of it being None/null

@@ -39,6 +41,7 @@ def defaultTrigger(self):
@property
def api_key(self) -> str:
return self._api_key
# return '1234'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean to leave this in and will make a commit removing it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this is fixed now

Comment on lines +71 to +140
try:
if self.st:
config = self.st.system.config()

devices = dict()
for d in config['devices']:
if not d['name']:
d['name'] = d['deviceID']
d['_shared_folders'] = {}
devices[d['deviceID']] = d

folders = dict()
for f in config['folders']:
if not f['label']:
f['label'] = f['id']
for d in f['devices']:
devices[d['deviceID']]['_shared_folders'][f['id']] = f
folders[f['id']] = f

matcher = Matcher(query.string)

# create device items
for device_id, d in devices.items():
device_name = d['name']

if match := matcher.match(device_name):
device_folders = ", ".join([f['label'] for f in d['_shared_folders'].values()])

actions = []
if d['paused']:
actions.append(
Action("resume", "Resume synchronization",
lambda did=device_id: self.st.system.resume(did))
)
else:
actions.append(
Action("pause", "Pause synchronization",
lambda did=device_id: self.st.system.pause(did))
)

item = StandardItem(
id=device_id,
text=f"{device_name}",
subtext=f"{'Paused ' if d['paused'] else ''}Syncthing device. "
f"Shared: {device_folders if device_folders else 'Nothing'}.",
iconUrls=self.iconUrls,
actions=actions
)

results.append(RankItem(item, match))

# create folder items
for folder_id, f in folders.items():
folder_name = f['label']
if match := matcher.match(folder_name):
folders_devices = ", ".join([devices[d['deviceID']]['name'] for d in f['devices']])
item = StandardItem(
id=folder_id,
text=folder_name,
subtext=f"Syncthing folder {f['path']}. "
f"Shared with {folders_devices if folders_devices else 'nobody'}.",
iconUrls=self.iconUrls,
actions=[
Action("scan", "Scan the folder",
lambda fid=folder_id: self.st.database.scan(fid)),
Action("open", "Open this folder in file browser",
lambda p=f['path']: openFile(p))
]
)
results.append(RankItem(item, match))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit try is meant to catch it when syncthing fails to initialize due to missing or invalid api key. Otherwise it spits errors with every character entered into the launcher. Plus, having it blank with no explanation outside of the console is not the most friendly design.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I maybe could do the try except only around the bit that checks if st is initialized/valid? So that it's a little less general

Comment on lines +142 to +156
if self._api_key == '':
item = StandardItem(
id=device_id,
text=f"{device_name}",
subtext=f"{'Paused ' if d['paused'] else ''}Syncthing device. "
f"Shared: {device_folders if device_folders else 'Nothing'}.",
iconUrls=self.iconUrls,
actions=actions
id="no_key",
text="Please enter your Syncthing API key in settings",
subtext="You can find your api key on your Syncthing web dashboard.",
iconUrls=self.iconUrls
)

results.append(RankItem(item, match))

# create folder items
for folder_id, f in folders.items():
folder_name = f['label']
if match := matcher.match(folder_name):
folders_devices = ", ".join([devices[d['deviceID']]['name'] for d in f['devices']])
else:
item = StandardItem(
id=folder_id,
text=folder_name,
subtext=f"Syncthing folder {f['path']}. "
f"Shared with {folders_devices if folders_devices else 'nobody'}.",
iconUrls=self.iconUrls,
actions=[
Action("scan", "Scan the folder",
lambda fid=folder_id: self.st.database.scan(fid)),
Action("open", "Open this folder in file browser",
lambda p=f['path']: openFile(p))
]
id="invalid_key",
text='Invalid API Key',
subtext=f"API Key {self._api_key} is invalid. Please try entering your API key again in settings.",
iconUrls=self.iconUrls
)
results.append(RankItem(item, match))

results.append(RankItem(item, 0))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in theory, this except should only fire if the api key is invalid or missing. But I can see how we might want the logic to be a little less broad. Basically rn it just checks if the key is blank, at which point it reports missing, else it reports invalid.

@ngencokamin
Copy link
Author

@ngencokamin lets continue the discussion here. what have you changed to make the config widget work?

Hey sorry for the slow reply, been crazy busy with work. Lemme run through and break down my changes and why I made them/what they do

@ngencokamin lets continue the discussion here. what have you changed to make the config widget work?

Oh I can also add better code comments if that'd be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants