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

Desktop: Stop watching external edits #1981

Merged
merged 6 commits into from
Oct 17, 2019
Merged

Conversation

CalebJohn
Copy link
Collaborator

My original intention was to start watching opened resources, but progress has been slow so I want to split it up into multiple pulls. This first pull slightly changes the file watching to watch for the close event. Currently all this means is that when a user closes an external editor they will see the icon return to normal.

The way I wrote the code here will hopefully pave the way for generalizing ExternalEditWatcher to support resources as well as notes.

@tessus tessus added the desktop All desktop platforms label Oct 13, 2019
@laurent22
Copy link
Owner

Looks good, if you could just have a look at the two comments I left it can be merged. Of course, the onClose event won't be called when using a non-modal editor, but it's still an improvement for modal ones.

What would be the purpose of watching resources? Is it to allow modifying them?

@CalebJohn
Copy link
Collaborator Author

What would be the purpose of watching resources? Is it to allow modifying them?

Basically yes, it will allow for explicit edits, but also there are other times you might want to sync. For example my pdf viewer saves some metadata on where the last place I viewed the file was. This means that when I open the document again my location will be saved. It would be nice to have this metadata syncing across the Joplin environments.

@laurent22
Copy link
Owner

Basically yes, it will allow for explicit edits, but also there are other times you might want to sync. For example my pdf viewer saves some metadata on where the last place I viewed the file was. This means that when I open the document again my location will be saved. It would be nice to have this metadata syncing across the Joplin environments.

That makes sense. Support for editable resources would be very useful although I think some code here and there expect resources to never change, so that would need to be looked at. I don't know if the ResourceFetcher service would handle modified resources automatically either or if it would have to be tweaked a bit.

@CalebJohn
Copy link
Collaborator Author

CalebJohn commented Oct 17, 2019

@laurent I also added localization to that error. I noticed that none of that logging is localized, should I quickly go in and add _() call's to those now that it's imported? Or is logging generally not localized?

Support for editable resources would be very useful although I think some code here and there expect resources to never change, so that would need to be looked at. I don't know if the ResourceFetcher service would handle modified resources automatically either or if it would have to be tweaked a bit.

This is good to know, I realized it was going to be a bit more work than I originally thought, which is why I decided to split it up :)

@laurent22
Copy link
Owner

Yes we don't localise log statements and errors that are unlikely to happen. It allows keeping the locale files relatively small. So if you could remove the _() from this error that would be great.

@CalebJohn
Copy link
Collaborator Author

@laurent22 Done!

@laurent22
Copy link
Owner

Great, thanks @CalebJohn!

@laurent22 laurent22 merged commit 0eb51e6 into laurent22:master Oct 17, 2019
@CalebJohn CalebJohn deleted the unwatch branch October 17, 2019 21:02
scoroi pushed a commit to scoroi/joplin that referenced this pull request Nov 10, 2019
…1981)

* Stop watching external edits

* move onClose to options

* Wrap openExternal in promise

* More specific error with locale

* Removed localization of external edit error
@laurent22
Copy link
Owner

@CalebJohn, I have reverted the change in this commit: 15f09ef

For me, I think it's only the "onClose" part that was causing a problem (because the launching process ends immediately) and not sure about the rest of the changes. If some of these changes can be restored, feel free to create a new pull request with them.

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

Successfully merging this pull request may close these issues.

3 participants