Skip to content

feat: Added Context Menu for markdown preview mode and copy url when … #3066

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.

Sign up for GitHub

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

Merged
Rokt33r merged 4 commits into from
Jul 22, 2019
Merged

feat: Added Context Menu for markdown preview mode and copy url when … #3066

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
090b5c3
feat: Added Context Menu for markdown preview mode and copy url when …
nathan-castlehow Jun 9, 2019
d9264c6
Fixed Test Fail Issue due to incorrect Require for context menu items
nathan-castlehow Jun 9, 2019
df976a2
Removed SetTimeout on Markdown Preview Context menu
nathan-castlehow Jul 3, 2019
4a52e2f
Removed Paste / Cut options from preview context menu as they are not…
nathan-castlehow Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion browser/components/CodeEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import styles from '../components/CodeEditor.styl'
const { ipcRenderer, remote, clipboard } = require('electron')
import normalizeEditorFontFamily from 'browser/lib/normalizeEditorFontFamily'
const spellcheck = require('browser/lib/spellcheck')
const buildEditorContextMenu = require('browser/lib/contextMenuBuilder')
const buildEditorContextMenu = require('browser/lib/contextMenuBuilder').buildEditorContextMenu
import TurndownService from 'turndown'
import {languageMaps} from '../lib/CMLanguageList'
import snippetManager from '../lib/SnippetManager'
Expand Down
33 changes: 4 additions & 29 deletions browser/components/MarkdownPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,20 @@ import mdurl from 'mdurl'
import exportNote from 'browser/main/lib/dataApi/exportNote'
import { escapeHtmlCharacters } from 'browser/lib/utils'
import yaml from 'js-yaml'
import context from 'browser/lib/context'
import i18n from 'browser/lib/i18n'
import fs from 'fs'
import { render } from 'react-dom'
import Carousel from 'react-image-carousel'
import ConfigManager from '../main/lib/ConfigManager'

const { remote, shell } = require('electron')
const attachmentManagement = require('../main/lib/dataApi/attachmentManagement')
const buildMarkdownPreviewContextMenu = require('browser/lib/contextMenuBuilder').buildMarkdownPreviewContextMenu

const { app } = remote
const path = require('path')
const fileUrl = require('file-url')

const dialog = remote.dialog

const uri2path = require('file-uri-to-path')

const markdownStyle = require('!!css!stylus?sourceMap!./markdown.styl')[0][1]
const appPath = fileUrl(
process.env.NODE_ENV === 'production' ? app.getAppPath() : path.resolve()
Expand Down Expand Up @@ -249,30 +245,9 @@ export default class MarkdownPreview extends React.Component {
}

handleContextMenu (event) {
// If a contextMenu handler was passed to us, use it instead of the self-defined one -> return
if (_.isFunction(this.props.onContextMenu)) {
this.props.onContextMenu(event)
return
}
// No contextMenu was passed to us -> execute our own link-opener
if (event.target.tagName.toLowerCase() === 'a' && event.target.getAttribute('href')) {
const href = event.target.href
const isLocalFile = href.startsWith('file:')
if (isLocalFile) {
const absPath = uri2path(href)
try {
if (fs.lstatSync(absPath).isFile()) {
context.popup([
{
label: i18n.__('Show in explorer'),
click: (e) => shell.showItemInFolder(absPath)
}
])
}
} catch (e) {
console.log('Error while evaluating if the file is locally available', e)
}
}
const menu = buildMarkdownPreviewContextMenu(this, event)
if (menu != null) {
setTimeout(() => menu.popup(remote.getCurrentWindow()), 30)
Copy link
Member

@ZeroX-DG ZeroX-DG Jun 15, 2019

Choose a reason for hiding this comment

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

Why do you need setTimeout here?

Copy link
Contributor Author

@nathan-castlehow nathan-castlehow Jun 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm great question. Copied this from the Code editor context menu, it has the same behaviour. Do we know why it was added in there? Might be worth just removing it in both places?

Copy link
Member

@ZeroX-DG ZeroX-DG Jun 16, 2019

Choose a reason for hiding this comment

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

That code was added in this PR: #2338. @ehhc can you explain why you need setTimeout there?

Copy link
Contributor

@ehhc ehhc Jun 16, 2019

Choose a reason for hiding this comment

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

To be honest, i can't remember.
Only thing i might imagine is, that setTimeout forces it to be in a separated thread --> not blocking the main gui...
But: there is a good chance, that that's bullshit...

Does it work without the timeout? If yes, feel free to remove it :)

Copy link
Contributor Author

@nathan-castlehow nathan-castlehow Jun 23, 2019

Choose a reason for hiding this comment

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

@ZeroX-DG thoughts?

Copy link
Member

@ZeroX-DG ZeroX-DG Jun 24, 2019

Choose a reason for hiding this comment

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

You heard from the master himself:

Does it work without the timeout? If yes, feel free to remove it :)
@ehhc - 2019

Copy link

@evanpetridis evanpetridis Jun 25, 2019

Choose a reason for hiding this comment

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

I have been hoping that this feature would be merged into the base branch. The fact that the original author can not remember why setTimeout was used is in itself a worry. If setTimeout is removed, the code may work in whatever preliminary testing that it gets, but may fail in some rare timing case in the future. 30ms is not a long time to wait for a menu to appear. I'd say leave it in and move on.

Copy link
Contributor

@ehhc ehhc Jun 25, 2019

Choose a reason for hiding this comment

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

@evanpetridis you're right, it is a worry. unfortunately, i cannot do anything about it anymore. Cannot force my way to remember things, i forgot. Nonetheless, at that time i worked with a lot of already existing code, modified it, deleted it, and added new code. All in all i did a lot of work concerning the attachment management of boostnote. I fear, in that context, it's possible to forget the reasons for some things. Even moren, when it's not even clear whether i added it or reused code, i found at other locations in the code base.
And i have to admit, that i, at that point in time and still, am not the expert concerning the technolgies used in boostnote. I hoped for a good codereview to find potential bugs but there is nothing more, i could do about it..

sorry if that's not what you wannted to hear.

Copy link

@evanpetridis evanpetridis Jun 26, 2019

Choose a reason for hiding this comment

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

@ehhc Hi ehhc, my message wasn't very clear. I think that you did the correct thing leaving that code where you did. Well done for actively contributing to open source software :). My point was that because we don't know why it is there, it's an unnecessary danger to remove it. It has no effect leaving it there but we have no idea what the effect of removing it is. I just hope that if the changes that @nathan-castlehow added get sufficiently commented before the changes get merged into the base branch.

Copy link
Contributor Author

@nathan-castlehow nathan-castlehow Jul 3, 2019

Choose a reason for hiding this comment

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

@ZeroX-DG i removed it from the preview context menu and it seems ok. I also removed the cut and paste options from the preview context menu as they aren't relevant.

}
}

Expand Down
65 changes: 64 additions & 1 deletion browser/lib/contextMenuBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import i18n from 'browser/lib/i18n'
import fs from 'fs'

const {remote} = require('electron')
const {Menu} = remote.require('electron')
const {clipboard} = remote.require('electron')
const {shell} = remote.require('electron')
const spellcheck = require('./spellcheck')
const uri2path = require('file-uri-to-path')

/**
* Creates the context menu that is shown when there is a right click in the editor of a (not-snippet) note.
Expand Down Expand Up @@ -62,4 +68,61 @@ const buildEditorContextMenu = function (editor, event) {
return Menu.buildFromTemplate(template)
}

module.exports = buildEditorContextMenu
/**
* Creates the context menu that is shown when there is a right click Markdown preview of a (not-snippet) note.
* @param {MarkdownPreview} markdownPreview
* @param {MouseEvent} event that has triggered the creation of the context menu
* @returns {Electron.Menu} The created electron context menu
*/
const buildMarkdownPreviewContextMenu = function (markdownPreview, event) {
if (markdownPreview == null || event == null || event.pageX == null || event.pageY == null) {
return null
}

// Default context menu inclusions
const template = [{
role: 'cut'
}, {
role: 'copy'
}, {
role: 'paste'
}, {
role: 'selectall'
}]

if (event.target.tagName.toLowerCase() === 'a' && event.target.getAttribute('href')) {
// Link opener for files on the local system pointed to by href
const href = event.target.href
const isLocalFile = href.startsWith('file:')
if (isLocalFile) {
const absPath = uri2path(href)
try {
if (fs.lstatSync(absPath).isFile()) {
template.push(
{
label: i18n.__('Show in explorer'),
click: (e) => shell.showItemInFolder(absPath)
}
)
}
} catch (e) {
console.log('Error while evaluating if the file is locally available', e)
}
}

// Add option to context menu to copy url
template.push(
{
label: i18n.__('Copy Url'),
click: (e) => clipboard.writeText(href)
}
)
}
return Menu.buildFromTemplate(template)
}

module.exports =
{
buildEditorContextMenu: buildEditorContextMenu,
buildMarkdownPreviewContextMenu: buildMarkdownPreviewContextMenu
}
14 changes: 13 additions & 1 deletion tests/lib/contextMenuBuilder.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ jest.mock('electron', () => {
})

const spellcheck = require('browser/lib/spellcheck')
const buildEditorContextMenu = require('browser/lib/contextMenuBuilder')
const buildEditorContextMenu = require('browser/lib/contextMenuBuilder').buildEditorContextMenu
const buildMarkdownPreviewContextMenu = require('browser/lib/contextMenuBuilder').buildMarkdownPreviewContextMenu

beforeEach(() => {
menuBuilderParameter = null
})

// Editor Context Menu
it('should make sure that no context menu is build if the passed editor instance was null', function () {
const event = {
pageX: 12,
Expand Down Expand Up @@ -124,3 +126,13 @@ it('should make sure that word suggestions creates a correct menu if there was a
expect(menuBuilderParameter[7].role).toEqual('selectall')
expect(spellcheck.getSpellingSuggestion).toHaveBeenCalledWith(wordToCorrect)
})

// Markdown Preview Context Menu
it('should make sure that no context menu is built if the Markdown Preview instance was null', function () {
const event = {
pageX: 12,
pageY: 12
}
buildMarkdownPreviewContextMenu(null, event)
expect(menuBuilderParameter).toEqual(null)
})