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: Add external editor actions to the note context menu. #2214

Merged
merged 5 commits into from
Jan 6, 2020

Conversation

miciasto
Copy link
Contributor

@miciasto miciasto commented Dec 22, 2019

Also start up external editor on note double click.

These changes enhance user experience by placing the actions where they feel natural.

Also start up external editor on note double click.

These changes enhance user experience by placing the actions where
they feel natural.
@miciasto
Copy link
Contributor Author

miciasto commented Dec 22, 2019

The proposed changes are to add external editor actions to the note context menu. In my experience, this is a place where the user might naturally expect to find them.

When right-click on a note in the note list, "Edit in external editor" is a new option. If the note is in the watching state already, then the new option is "Stop watching external editor". If two or more notes are selected, then the option is unavailable.

In addition, double-click on the note in the note list launches the external editor. If the note is in the watching state, then double click does nothing.

I could not find tests relevant to this part of the application. Please advise if there are test cases to be updated.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

I like the change, but I can't recall this being discussed in the forum.
Let's see what Laurent says.

ElectronClient/app/gui/NoteList.jsx Outdated Show resolved Hide resolved
@tessus tessus added the desktop All desktop platforms label Dec 23, 2019
@laurent22
Copy link
Owner

Looks good but I prefer we reserve the use of double-click for now, as not everyone has an external editor configured. I'd expect double-clicking opens the note in a new window, not necessarily in an external editor.

Changes in response to review comments.
@miciasto
Copy link
Contributor Author

Agreed. Will remove the double-click.

@miciasto
Copy link
Contributor Author

miciasto commented Dec 29, 2019

While testing this, I found a pre-existing bug in the context menu; when you right-click on a note in the list, the context menu is shown for the note under the mouse, but the action is applied to the currently selected note.

Perhaps right-clicking a note in the list should also select it if it isn't already selected? This probably needs discussion, should I create a separate issue for that?

Edit: I tried on an older version, and it behaves differently. I'll look into it further.

This is to ensure correct behaviour even when the user launches the
action on a note in the list that is under the pointer, but not selected.
@miciasto
Copy link
Contributor Author

miciasto commented Dec 29, 2019

There was a bug (as described above) and I have fixed it for this feature in the last commit.

To fix it, I moved the launching of the external editor to the main screen object, because the current note may not be the one that is being acted upon.

The same bug exists for the pdf export and maybe other actions in the context menu, I haven't checked them all. I propose to create an issue to fix those separately.

@tessus
Copy link
Collaborator

tessus commented Dec 29, 2019

To fix it, I moved the launching of the external editor to the main screen object, because the current note may not be the one that is being acted upon.

I am not sure if this was necessary. e.g. Delete works and is not in the main screen object either.
I think this should be fixed differently.

But export to pdf has the issue you described. This certainly should be fixed.

@miciasto
Copy link
Contributor Author

miciasto commented Dec 29, 2019

I am not sure if this was necessary. e.g. Delete works and is not in the main screen object either.
I think this should be fixed differently.

Yes, that is correct.

Though perhaps the main difference is that DeleteNote happens completely within the note list, while the other actions are also generated from the toolbar and main menu. So DeleteNote does not use a message.

I can implement the fix in NoteList or NoteListUtils. Would that be acceptable? I'm still not 100% across the architecture so am grateful for any guidance.

@miciasto
Copy link
Contributor Author

For reference, I used the Add remove tags as the model for the fix, which is implemented in the MainScreen.

@miciasto
Copy link
Contributor Author

I have reverted the changes made in MainScreen and re-implemented the fix back in the NoteList.

@miciasto miciasto requested review from tessus and laurent22 December 30, 2019 21:25
@miciasto
Copy link
Contributor Author

miciasto commented Jan 4, 2020

Please see #2255 prior to this. If it is acceptable, then I would like to update this before review, to use the same method.

@laurent22
Copy link
Owner

Looks good, thanks @mic704b!

@laurent22 laurent22 merged commit 42ada71 into laurent22:master Jan 6, 2020
@miciasto
Copy link
Contributor Author

miciasto commented Jan 7, 2020

Thank you!

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