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

Favorites plugin - clicking on items in Favorites does not open them #9280

Closed
jb261 opened this issue Nov 11, 2023 · 12 comments
Closed

Favorites plugin - clicking on items in Favorites does not open them #9280

jb261 opened this issue Nov 11, 2023 · 12 comments
Assignees
Labels
bug It's a bug desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system

Comments

@jb261
Copy link

jb261 commented Nov 11, 2023

Operating system

Windows

Application version

2.13.5

Desktop: About dialog content

Favorites plugin (v1.3.0) isn't working with 2.13.5. Left clicking on any of the notes in Favorites does not open the note; right clicking does work as expected though.

Joplin 2.13.5 (prod, win32)

Sync Version: 3
Profile Version: 44
Keychain Supported: Yes

Revision: 1d04ec6

Current behaviour

Left clicking on a note in Favorites does not open the note.

Expected behaviour

Joplin should open the note.

Logs

No response

@placoderm
Copy link

I can confirm the same behaviour, also on Windows with the same Joplin and same plugin version.

@laurent22 laurent22 added desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system v2.13 bug It's a bug labels Nov 11, 2023
@laurent22
Copy link
Owner

Ok that's a tricky one. The favorite plugin had a click handler on a disabled input field, which indeed I would expect doesn't work, however somehow it did with previous Electron versions and doesn't anymore:

image

I tried reverting to the previous Electron version we used, v19 and it works fine. @personalizedrefrigerator, I wonder if we should revert to an earlier Electron version, which doesn't have this problem, to give us time to address this properly?

For info, as a test, I've reverted the upgrade in this PR: #9285

Another issue probably caused by the Electron upgrade is this one: #8788

@laurent22
Copy link
Owner

@jb261, @placoderm, as a temporary workaround, you can click outside of that invisible input field - for example to the left of the icon, and that would work

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Nov 12, 2023

I tried reverting to the previous Electron version we used, v19 and it works fine. @personalizedrefrigerator, I wonder if we should revert to an earlier Electron version, which doesn't have this problem, to give us time to address this properly?

Let's do v25 and not v19. The Favorites plugin seems to work for me in v2.13.4, which uses Electron 25. v2.13.5 is the first version that uses Electron 26.

Downgrading to Electron 19 may cause regressions in other plugins (e.g. it causes some minor style issues with the Freehand Drawing plugin and might not support the :has selector).

@placoderm
Copy link

you can click outside of that invisible input field - for example to the left of the icon, and that would work

LOL. Yes that works. For me I don't have the icons (I must have done some css hiding?) but if I click on the very edge of the favorite tab/button then it does indeed work. Thanks!!

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Nov 13, 2023

As its author hasn't been active on GitHub in over a year, we will likely need to fix this issue without updating the original plugin (I've opened a pull request in case @benji300 is still maintaining it). Here are some ideas about how this might be done.

If the plugin is unmaintained

Idea 1: Update by committing directly to the plugin repository

We might be able to commit a new JPL file and manifest for the plugin directly to the plugin repository.

If we clear _npm_package_name in the manifest when committing, this will prevent the plugin from being reverted to a previous version.

Idea 2: Transfer the Favorites plugin to a separate NPM package/GitHub repository and update it there

This would require updating the plugin bot to support overriding one plugin's NPM package with another. Here's how this might work:

  1. Create a new pluginOverrides.json file in the plugin repository.
    • This file could map NPM package names to replacement NPM package names
      • Alternatively, we could use the existing _npm_package_name field in manifestOverrides.json for this
      • We could require that replacement package names start with @joplin/ (or a similar namespace), though I'm not sure this would be necessary.
      • Regardless, override packages could have a different prefix (not joplin-plugin-) so that they don't show up in the standard search results.
  2. Modify packages/plugin-repo-cli/index.ts/processNpmPackage to check pluginOverrides.json for an overridden package name before downloading a package from NPM. If an override exists, download and extract the package override instead of the original.
  3. Add a special case to checkIfPluginCanBeAdded.ts to allow overriding an original package if the new package is marked as an allowed override for the original package.

Idea 3: Update joplin-desktop to apply workaround CSS to the favorites plugin sidebar

We could update joplin-desktop to apply additional CSS to either all plugin sidebars or just the favorites plugin sidebar. A bug report related to this issue suggests input[disabled] { pointer-events: none; } (which may not be without side-effects).

@placoderm
Copy link

I realize it may be too complicated, but how about moving the plugin's functionality into core? This seems to be a good candidate for adding.

@laurent22
Copy link
Owner

I realize it may be too complicated, but how about moving the plugin's functionality into core? This seems to be a good candidate for adding.

Adding plugins to core is something we want to support eventually, but in the short term we need to fix that particular plugin for this release so this is what we're investigating at the moment.

But the favorite plugin would indeed make a good candidate for a default bundled plugin.

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Nov 13, 2023

I realize it may be too complicated, but how about moving the plugin's functionality into core? This seems to be a good candidate for adding.

It might be — the feature freeze is just a few days away however, and we would likely need to make the following changes:

  • Update the plugin's code style to match Joplin's
  • Switch from the plugin API to internal APIs (maybe? Depends on how we do this.)
  • Make the plugin localizable
  • Fix keyboard navigation/accessibility issues

After version 2.13 is released, I plan to work on #7934 (including a set of default plugins with the app). Plugins would still be plugins, and thus not included directly in core, but a version of each plugin could be bundled with the application. (So hopefully we'll have that in version 2.14!)

@benji300
Copy link

@personalizedrefrigerator, @laurent22: I can fix the plugin by accepting the PR and create a release (I hope my local environment still works). I think tomorrow I can spend some time to do so...

But for future changes I would need someone to maintain the plugins. As you recognized, I don't have time anymore to maintain this and the other plugins.

I am also happy to transfer the repositories to another location in GitHub. What about moving them to the Joplin organization? The plugins could then be maintained and further developed by the community.

But I'm not quite sure what to do with the npm packages...

@placoderm
Copy link

I can fix the plugin by accepting the PR and create a release

Just want to say thank you for creating the plugin. And thanks for helping to get it working with the new release.

@laurent22
Copy link
Owner

@benji300, thanks for getting back to us! If you could transfer the GitHub repo indeed that would be great, or add some of us as administrator (@tessus and/or laurent22 should be good enough).

The more important part for deployment would be the npm package - would you be able to transfer it to us? It's described here how to do this: https://docs.npmjs.com/transferring-a-package-from-a-user-account-to-another-user-account. The organisation is @joplin, or maybe laurent22 if a username is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system
Projects
None yet
Development

No branches or pull requests

5 participants