Desktop: preview mode fix #2576

Runo-saduwa · 2020-02-24T20:29:16Z

DESCRIPTION:
This patch is a fix for the issue #2467 . Previously before this solution, the Edit menu items were not disabled on PREVIEW, this caused users to be able to use short cuts such as CTRL+B, CTRL+` on PREVIEW mode which is an unwanted behavior.

HOW TO TEST
Use the following test cases to confirm this solution::

Add a note to a notebook and click the layout toggler to PREVIEW:
EXPECTED RESULT: 'copy', 'paste', 'cut', 'select all', 'bold', 'italic', 'link', 'code', 'insert Date Time', 'Start External Editing', 'show Local Search' should be disabled
Add a note to a notebook and click the layout toggler to EDITOR:
EXPECTED RESULT: 'copy', 'paste', 'cut', 'select all', 'bold', 'italic', 'link', 'code', 'insert Date Time', 'Start External Editing', 'show Local Search' should be enabled
Add a note to a notebook and click the layout toggler to EDITOR AND PREVIEW MODE:
EXPECTED RESULT: 'copy', 'paste', 'cut', 'select all', 'bold', 'italic', 'link', 'code', 'insert Date Time', 'Start External Editing', 'show Local Search' should be enabled
Imported a HTML note and click the layout toggler to EDITOR/PREVIEW MODE/SPLIT:
EXPECTED RESULT: 'copy', 'paste', 'cut', 'select all', 'bold', 'italic', 'link', 'code', 'insert Date Time', 'Start External Editing', 'show Local Search' should be disabled
CREATE an Empty Notebook (book without notes)
EXPECTED RESULT: 'copy', 'paste', 'cut', 'select all', 'bold', 'italic', 'link', 'code', 'insert Date Time', 'Start External Editing', 'show Local Search' should be disabled

ElectronClient/app.js

miciasto · 2020-02-24T22:08:15Z

@Runo-saduwa nice work, didn't check the logic, just a few style comments.

Small images of the relevant part of the screen would be nice, in the PR description, maybe showing before and after. The words have all the details, but pictures are sometimes clearer.

tessus

The problem with HTML notes is still not fixed.

HTML notes must not enable any of the menu items - no matter the viewing mode. You are very close. You almost got it.

Runo-saduwa · 2020-02-25T20:44:23Z

I am still trying to set up Web Clipper to test this. as soon as i successfully set up Web Clipper on my browser, this issue will be resolved

tessus · 2020-02-26T03:26:59Z

Just use the "normal" web clipper with your production Joplin insgtance. Then create a HTML note as I explained earlier, export it (jex), and import it into your dev Joplin instance.

tessus · 2020-02-26T03:34:12Z

Or use this one:

html-test.zip

Runo-saduwa · 2020-02-26T04:16:12Z

Thank You, I will give updates shortly

Runo-saduwa · 2020-02-26T18:20:12Z

Just use the "normal" web clipper with your production Joplin insgtance. Then create a HTML note as I explained earlier, export it (jex), and import it into your dev Joplin instance.

Special thanks to @tessus

Runo-saduwa · 2020-02-26T18:27:14Z

@PackElend Please label me, thanks.

tessus

Apart from the empty line @mic704b mentioned, the PR looks good now.

Please delete the empty line.

tessus

LGTM

Runo-saduwa · 2020-02-27T05:14:57Z

Thanks for the quick review @tessus

ElectronClient/app.js

PackElend · 2020-02-29T11:11:29Z

@Runo-saduwa can you summarize you changes?
Please also include the comments / reasons.
you can do directly in the commit

This PR has a lot of comments, so it is quite challenging to follow it.
thank you

Runo-saduwa · 2020-02-29T11:53:04Z

ElectronClient/app.js

@@ -1178,7 +1181,8 @@ class Application extends BaseApplication {
 for (const itemId of ['copy', 'paste', 'cut', 'selectAll', 'bold', 'italic', 'link', 'code', 'insertDateTime', 'commandStartExternalEditing', 'showLocalSearch']) {
 const menuItem = Menu.getApplicationMenu().getMenuItemById(`edit:${itemId}`);
 if (!menuItem) continue;
- menuItem.enabled = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_MARKDOWN;
+ const isHtmlNote = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML;


So full condition should be const isHtmlNote = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML;

based on this comment by Laurent, i have changed the code to use this. which will return true if its a HTML note and false if its not.

const isHtmlNote = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML

ElectronClient/app.js

Runo-saduwa · 2020-02-29T11:56:48Z

ElectronClient/app.js

@@ -1178,7 +1181,8 @@ class Application extends BaseApplication {
 for (const itemId of ['copy', 'paste', 'cut', 'selectAll', 'bold', 'italic', 'link', 'code', 'insertDateTime', 'commandStartExternalEditing', 'showLocalSearch']) {
 const menuItem = Menu.getApplicationMenu().getMenuItemById(`edit:${itemId}`);
 if (!menuItem) continue;
- menuItem.enabled = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_MARKDOWN;
+ const isHtmlNote = !!note && note.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML;
+ menuItem.enabled = !isHtmlNote && layout !== 'viewer' && !!note;


Then i modified the logic for enabling and disabling the menuItem with this condition
!isHtmlNote && layout !== 'viewer' && !!note
which causes the same side-effects as this:

if(isHtmlNote || layout === 'viewer' || !note){ menuItem.enabled = false; } else { menuItem.enabled = true; }

tessus

Thanks, I think it now includes all the changes Laurent requested.

tessus · 2020-03-06T01:12:01Z

Anything left to do here or can we merge this?

laurent22 · 2020-03-06T22:59:36Z

We got there eventually! Thanks @Runo-saduwa for the fix and everyone for reviewing.

…only mode (#2576)" Some items still need to be enabled so implementation needs to be reviewed. Reverting for now. This reverts commit 0be982c.

preview mode fix

bb3039b

miciasto reviewed Feb 24, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

miciasto reviewed Feb 24, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

miciasto reviewed Feb 24, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

tessus reviewed Feb 25, 2020

View reviewed changes

tessus linked an issue Feb 25, 2020 that may be closed by this pull request

edit menu items and shortcuts not disabled in preview mode #2467

Closed

preview-edit mode fixed

8dd59bb

PackElend added the gsoc-2020 label Feb 26, 2020

tessus reviewed Feb 27, 2020

View reviewed changes

tessus added PR-tested desktop All desktop platforms labels Feb 27, 2020

remove unecessary white space

480c23f

tessus approved these changes Feb 27, 2020

View reviewed changes

laurent22 requested changes Feb 27, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

ElectronClient/app.js Outdated Show resolved Hide resolved

ElectronClient/app.js Outdated Show resolved Hide resolved

improved logic and removed empty line

7e6b6c2

Runo-saduwa force-pushed the preview-mode-fix branch from cd09be3 to 7e6b6c2 Compare February 27, 2020 12:04

improved code and removed comment

545a8cd

Runo-saduwa force-pushed the preview-mode-fix branch from dbf19bc to 545a8cd Compare February 27, 2020 20:33

laurent22 reviewed Feb 27, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

Runo-saduwa added 2 commits February 27, 2020 22:25

improved code again

2cecbe7

changed note type check logic

713f8ae

laurent22 reviewed Feb 29, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

improved code

d9259f8

Runo-saduwa commented Feb 29, 2020

View reviewed changes

ElectronClient/app.js Show resolved Hide resolved

Runo-saduwa commented Feb 29, 2020

View reviewed changes

tessus reviewed Feb 29, 2020

View reviewed changes

tessus added the reviewed-todo label Mar 2, 2020

laurent22 merged commit 0be982c into laurent22:master Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: preview mode fix #2576

Desktop: preview mode fix #2576

Runo-saduwa commented Feb 24, 2020 •

edited

Loading

miciasto commented Feb 24, 2020 •

edited

Loading

tessus left a comment •

edited

Loading

Runo-saduwa commented Feb 25, 2020

tessus commented Feb 26, 2020

tessus commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

tessus left a comment

tessus left a comment

Runo-saduwa commented Feb 27, 2020

PackElend commented Feb 29, 2020 •

edited

Loading

Runo-saduwa Feb 29, 2020 •

edited

Loading

Runo-saduwa Feb 29, 2020

tessus left a comment

tessus commented Mar 6, 2020

laurent22 commented Mar 6, 2020

Desktop: preview mode fix #2576

Desktop: preview mode fix #2576

Conversation

Runo-saduwa commented Feb 24, 2020 • edited Loading

miciasto commented Feb 24, 2020 • edited Loading

tessus left a comment • edited Loading

Choose a reason for hiding this comment

Runo-saduwa commented Feb 25, 2020

tessus commented Feb 26, 2020

tessus commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

Runo-saduwa commented Feb 26, 2020

tessus left a comment

Choose a reason for hiding this comment

tessus left a comment

Choose a reason for hiding this comment

Runo-saduwa commented Feb 27, 2020

PackElend commented Feb 29, 2020 • edited Loading

Runo-saduwa Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Runo-saduwa Feb 29, 2020

Choose a reason for hiding this comment

tessus left a comment

Choose a reason for hiding this comment

tessus commented Mar 6, 2020

laurent22 commented Mar 6, 2020

Runo-saduwa commented Feb 24, 2020 •

edited

Loading

miciasto commented Feb 24, 2020 •

edited

Loading

tessus left a comment •

edited

Loading

PackElend commented Feb 29, 2020 •

edited

Loading

Runo-saduwa Feb 29, 2020 •

edited

Loading