feat: Added Context Menu for markdown preview mode and copy url when … #3066
Conversation
|
I was wondering why there was no 'Copy Url' on that context menu. Great addition. Hope it makes it through review. |
| } | ||
| const menu = buildMarkdownPreviewContextMenu(this, event) | ||
| if (menu != null) { | ||
| setTimeout(() => menu.popup(remote.getCurrentWindow()), 30) |
There was a problem hiding this comment.
Why do you need setTimeout here?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
You heard from the master himself:
Does it work without the timeout? If yes, feel free to remove it :)
@ehhc - 2019
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
ZeroX-DG
added
awaiting changes 🖊️
Rokt33r
removed
the
approved 👍
nathan-castlehow
deleted the
feat-add-copy-url-option-in-markdown-preview
branch
|
This PR conflicts with the switch preview setting 'on right click'. See #3144. |
6 participants
Description
Apologies in advance if i have missed steps here. First time contributing to this repo.
This adds a context menu to the Markdown Preview mode with the usual options + the new Copy Url option when right clicking a hyperlink.
Issue fixed
#3058
Type of changes
Checklist: