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

Android: adds "recent notes" widget #5571

Closed
wants to merge 2 commits into from
Closed

Conversation

fstanis
Copy link
Contributor

@fstanis fstanis commented Oct 14, 2021

This is effectively a rebase and update of #3458.

Link to proposal

This PR adds a simple widget that displays up to 10 most recently opened notes:

image

Changes from #3458:

  • Logic to handle deep links such as joplin://notes/... and joplin://folder/... is included, so the notes in the widget are clickable and working. Based on roman-r-m/joplin@3b36f6e - thanks @roman-r-m.
  • Instead of manually maintaining the list of recent notes, it now queries the database for top 10 notes by updated_time.

@@ -764,6 +769,8 @@ class AppComponent extends React.Component {

await setupNotifications(this.props.dispatch);

await updateRecentsWidget();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is incorrect - I added it for testing purposes only. Instead of updating only on start, the list of recents should be updated every time there's a meaningful change (sync, modification).

@laurent22 any thoughts on how best to do this? Would reduxSharedMiddleware be a good place to do this?

Copy link
Owner

Choose a reason for hiding this comment

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

I think you could listen to eventManager.on('itemChange', ....), but check that the item is a note, and also add a few seconds delay as there could be hundreds of these events during sync. i.e. start a timer, then cancel it if you receive another event, and start it again, and so on until things are quiet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, this sounds like a good approach. I also like the debounce mechanism you mentioned, I'll implement that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this in the latest commit.

One bug I noticed: if the user modifies a note and then immediately goes to the home screen, the update won't fire (because the app is no longer in the foreground when the timeout expires), at least not until the app is brought back to the foreground. The only solution that comes to mind is to use something like headless JS, but this honestly sounds like an overkill.

@roman-r-m
Copy link
Collaborator

Logic to handle deep links such as joplin://notes/... and joplin://folder/... is included, so the notes in the widget are clickable and working. Based on roman-r-m/joplin@3b36f6e - thanks @roman-r-m.

You may want to check #5168 and adjust the URL format (even though I personally don't like this x-callback-url stuff)

@tessus tessus added the android label Oct 17, 2021
packages/app-mobile/root.tsx Outdated Show resolved Hide resolved
packages/app-mobile/root.tsx Outdated Show resolved Hide resolved
packages/app-mobile/root.tsx Outdated Show resolved Hide resolved
@fstanis
Copy link
Contributor Author

fstanis commented Nov 20, 2021

I split the relevant URL handling logic into #5764 - this PR is also ready for review, but tapping on notes in the widget won't work until #5764 is merged.

@laurent22
Copy link
Owner

laurent22 commented Dec 28, 2021

If we release this pull request, I wonder if we should mark the feature as beta for now? I feel there's a bit more that would need to be done to be considered production ready, for example by making the UI nicer, providing more options such as which notebook to display, how many notes, etc.

And doing all this in this pull request would be too much, so I think a good compromise would be to make it clear that it's not quite finished. Would you be ok with this, and if so could you add the "Beta" string where relevant? (maybe at the point where the widget is added)

if (itemType !== ModelType.Note) {
return;
}
updateRecentsWidgetWithDebounce();
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way to detect if the widget is installed, and if it's not, to skip this call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems the only way is to store the widget status in shared preferences and use that. However, I don't think this is a good idea:

  • It complicates the update flow, as the widget would have no data when first added, so we'd need to expose some way for the widget to trigger these queries and I'm not sure how to cleanly do that.
  • I'm not sure if we're saving much, if anything, since React would still need to use native code to read/write shared preferences just to check if a widget is active.

@laurent22
Copy link
Owner

Not sure what's the status of this pull request?

@fstanis
Copy link
Contributor Author

fstanis commented Jan 9, 2022

Picking it up later this this month, likely this week. Agree on marking it as beta.

@fstanis
Copy link
Contributor Author

fstanis commented Feb 6, 2022

Updated #5764 - waiting for that to be merged to continue working on this.

@laurent22
Copy link
Owner

@fstanis, sorry for taking so long to get back to you. I've been thinking whether it makes sense to add this feature or not, and I have unfortunately decided I'd rather not. The reason is that I struggle a lot to release new mobile versions (to the point I can't even release 2.9 at all this month), and with more native code and complexity it will reach a point where we can't manage the mobile apps anymore.

I appreciate the time you spent on this and wish I could merge, but I'd rather not at this point. Your code may be used as a reference though if we ever decide to add this back.

@laurent22 laurent22 closed this Dec 21, 2022
Repository owner locked as resolved and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants