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

Add context menu #2001

Merged
merged 15 commits into from Nov 3, 2017

Conversation

Projects
None yet
9 participants
@rickycodes
Contributor

rickycodes commented Jul 13, 2017

closes: #265

First stab at creating a right-click context menu...

right-click-copy

/cc @leo

@rickycodes rickycodes changed the title from Add context menu (WIP) to Add context menu Jul 14, 2017

@timothyis timothyis requested a review from chabou Jul 14, 2017

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 15, 2017

Looking at the original issue, I saw #265 (comment) and decided to add two items to the context menu:

  • 'Open Selection as URL'
  • 'Search the Web for Selection'

selectionMenu

/cc @CodeTheory

@timothyis timothyis requested a review from matheuss Jul 15, 2017

@timothyis

This comment has been minimized.

Member

timothyis commented Jul 15, 2017

This is incredible! Can't wait to have this in! 🎉
cc @matheuss @rauchg @chabou @ppot

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 15, 2017

I ended up just using .concat to add the two new items to the end of the menu since splice is problematic (modifies the original) 😓

screenshot for context:
screen shot 2017-07-15 at 12 43 00 pm

@henrikdahl henrikdahl self-requested a review Jul 16, 2017

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Jul 16, 2017

This does and will clash with

// if true, on right click selected text will be copied or pasted if no
    // selection is present (true by default on Windows)
    quickEdit: true,

so I guess having it enabled should disable the menu or the other way round

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Jul 16, 2017

Same with

    // set to `true` (without backticks) if you're using a Linux setup that doesn't show native menus
    // default: `false` on Linux, `true` on Windows (ignored on macOS)
    showHamburgerMenu: '',

in the case of Windows or true the right click menu should probably adjust itself to not duplicate items with the hamburger menu

clear buffer command works but is still bugged on Windows (#1808 (comment))
cut undo select all and redo do not work. nothing happens.

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 16, 2017

cut undo select all and redo do not work. nothing happens.

do these items behave as expected or differently when using the main menus? (I haven't really been able to get them to work in either case), they should perform the same as the other menus (since they're just duplicated from there)

re: windows settings, I will tweak what I have to make sure there's no conflicts (and test a bit in windows) to see what makes the most sense.

/cc @Stanzilla

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Jul 16, 2017

Yeah, they don't work from the main menu either. Do they work on any OS?

enabled: !isCollapsed
},
{
label: 'Search the Web for Selection',

This comment has been minimized.

@henrikdahl

henrikdahl Jul 16, 2017

Collaborator

Search with Google is more specific and consistent with other context menus ✌️

This comment has been minimized.

@timothyis

timothyis Jul 16, 2017

Member

Is there anyway to open a link with the users default search engine? That would be awesome.
Default search engine probably would come from the default browser > default search engine

This comment has been minimized.

@rickycodes

rickycodes Jul 16, 2017

Contributor

Could leave it as Google by default and make it configurable via .hyper.js?

This comment has been minimized.

@rickycodes

rickycodes Jul 16, 2017

Contributor

both terminal and iTerm2 have Search with Google

This comment has been minimized.

@timothyis

timothyis Jul 17, 2017

Member

Yeah, I'd like that. I know a few people who'd like not to use Google.

This comment has been minimized.

@henrikdahl

henrikdahl Jul 17, 2017

Collaborator

With Mac OS services you should be able to search with default browser and engine but most likely unusable since Hyper also supports other operative systems ✌️

This comment has been minimized.

@rickycodes

rickycodes Jul 17, 2017

Contributor

in b3a9d30 I've made this configurable via exposing a new key/value (searchEngineUrl) from config so this can be customized via .hyper.js

defaults to google, but can be anything the user desires.

this would likely need to be addressed in app/config/config-default.js as well (similar to bellSoundURL)

I left the wording as is.

/cc @henrikdahl @CodeTheory

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 16, 2017

Yeah, they don't work from the main menu either. Do they work on any OS?

this is a good question, I'm not sure under what circumstances they are designed to work... the main thing is that this is not a regression on my part 😉

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 22, 2017

I nixed the url/google functionality (as I think that should be handled in a different PR, or possibly a plugin?)

@Stanzilla this should account for the items you mentioned quickEdit and showHamburgerMenu. let me know if this makes sense

/cc @CodeTheory

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Jul 22, 2017

showHamburgerMenu is a bit problematic, the default is '' which is sort of auto, that means it is actually true on Windows even if it is not set to true in the config. That is a problem because a Windows user can't set it to falsewhen he would prefer using your context menu over the hamburger and also makes you checking for it kinda useless since it will never be false on Windows.

My concern was to not duplicate menu entries when both hamburger and context are enabled but since you removed the URL/Google option it's 100% the same anyway, no?

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 22, 2017

@Stanzilla what do you think makes the most sense in your opinion? should I just drop the showHamburgerMenu check? I'm not entirely sure what makes the most sense for this as I am not a windows user

maybe I'll fire up a windows VM and see 👁

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Jul 22, 2017

I'd drop the check, yeah. Makes the most sense to me.

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Jul 29, 2017

@Stanzilla I dropped that showHamburgerMenu

lmk if this makes sense

@ppot

This comment has been minimized.

Collaborator

ppot commented Aug 2, 2017

@rickycodes Are you binding the menu context with keymaps?

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Aug 2, 2017

@ppot I'm just building the context menu from what already exists (combining edit + shell .submenu's here: https://github.com/zeit/hyper/pull/2001/files#diff-ac33cd5e16269e3f364363f9865b9ff5R24)

so yea, they have the same role/accelerator properties, if that's what you mean?

@rickycodes rickycodes referenced this pull request Aug 4, 2017

Closed

Add context menu #265

const contextMenuTemplate = () => {
const seperator = {type: 'separator'};
return editMenu(commands).submenu
.concat(seperator, shellMenu(commands, createWindow).submenu);

This comment has been minimized.

@ppot

ppot Aug 5, 2017

Collaborator

you are using it once. no need for const there
simply use {type: 'separator'}

@ppot

This comment has been minimized.

Collaborator

ppot commented Aug 5, 2017

@rickycodes The problem that I see is that, since you expose the menu to plugins. They will update the base menu instead of the contextMenu. You should make a copy of the menu and then add your own items to it and those of plugins.

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Aug 5, 2017

@ppot I'm using .concat, so it actually is a copy already:

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/concat

The concat() method is used to merge two or more arrays. This method does not change the existing arrays, but instead returns a new array.

since the copy is all that's exposed from menu.js there's no way to alter the original .submenus, all that can be altered is the contextMenuTemplate

@chabou

This comment has been minimized.

Collaborator

chabou commented Aug 13, 2017

Sorry for the delay, I was on vacation.
Amazing job here!! 😍

Reviewing it right now

};
// expose to plugins
app.contextMenuTemplate = contextMenuTemplate();

This comment has been minimized.

@chabou

chabou Aug 13, 2017

Collaborator

It could be better to have same way than menu, to decorate it from plugins:
Plugins export a decorateContextMenu function which is called when app start like here

Or maybe call a plugin exported function when context menu will show for a given term. With this function, plugin can (un)check some item according to its internal state for this term. Very useful for plugin like hyper-broadcast or for a plugin that will let user define term style directly from context menu 😍

e.preventDefault();
e.stopPropagation();
const {quickEdit} = config.getConfig();

This comment has been minimized.

@chabou

chabou Aug 13, 2017

Collaborator

You dont have to import/check config. You should check this.props.quickEdit like here

const contextMenu = Menu.buildFromTemplate(contextMenuTemplate);
if (show) {
// throttle popup so right click has time to highlight text
setTimeout(() => contextMenu.popup(getCurrentWindow()), 100);

This comment has been minimized.

@chabou

chabou Aug 13, 2017

Collaborator

I am really not fan to call this here.
You are right, context menu should be contextual to right-clicked term.
But I would prefer to dispatch a redux action (and maybe reduce it to retain in state which term is concerned), make a rpc call and popup context menu from app.

Term is a component and should not have any knowledge of electron.

This comment has been minimized.

@rickycodes

rickycodes Aug 14, 2017

Contributor

Term is a component and should not have any knowledge of electron.

ya 😅

i had wondered about this

I'll take a look at setting this up

This comment has been minimized.

@chabou

chabou Aug 15, 2017

Collaborator

Do not hesitate to ask question in this PR or directly on slack!
Really happy to help you on this awesome feature!

This comment has been minimized.

@rickycodes

rickycodes Aug 18, 2017

Contributor

@chabou I've made some changes that add a redux action + state/reduce

it's currently toggling the contextMenu as an effect in the actions, which I don't think is quite right 🤔

make a rpc call and popup context menu from app

could you maybe elaborate on that, where ideally would you like to see this/what are your thoughts on setting that up?

This comment has been minimized.

@chabou

chabou Aug 20, 2017

Collaborator

It could be better to make a rpc.emit in this effect().
contextMenu.popup should occur in MainProcess not in the renderer.

@chabou

This comment has been minimized.

Collaborator

chabou commented Aug 20, 2017

You can check what has been done for HamburgerMenu. ContextMenu is quite the same (for showing).

@rickycodes rickycodes changed the base branch from master to canary Oct 13, 2017

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Oct 13, 2017

@chabou this has now been rebased over zeit:canary and is now targeting it

I had to make a couple adjustments to accommodate the new changes

lmk what you think

const contextMenuTemplate = createWindow => {
return editMenu(commands).submenu.concat({type: 'separator'}, shellMenu(commands, createWindow).submenu);
};

This comment has been minimized.

@rickycodes

rickycodes Oct 14, 2017

Contributor

maybe an alternative to this:

const contextMenuTemplate = createWindow => {
  const shell = shellMenu(commands, createWindow).submenu
  const edit = editMenu(commands).submenu
  return edit.concat({type: 'separator'}, shell);
};
@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Oct 20, 2017

@chabou have you had a chance to look at this? I've actually made some refinements, and have an example where the menu changes/updates based on selection... in this version I've made it so cut and copy are only available if text is selected... text selection also expands two new options: Open Selection as URL and Search the Web for Selection:

hyper

you can compare the changes here: rickycodes/hyper@rickycodes:issue-265...add-context-menu

lmk what you think!

@chabou

Please remove all right click mentions

@@ -64,6 +64,13 @@ export default class Terms extends Component {
this.terms[uid] = term;
}
componentDidMount() {
window.addEventListener('contextmenu', () => {

This comment has been minimized.

@chabou

chabou Oct 29, 2017

Collaborator

I didn't know/see there was this event.
In this case, please remove any mention of right click notion. We should only speak about context menu.

@chabou

This comment has been minimized.

Collaborator

chabou commented Oct 29, 2017

I love the idea to pass selection in redux action.
Please make it happen 🙏

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Oct 30, 2017

@chabou I've made the requested changes

@chabou

This comment has been minimized.

Collaborator

chabou commented Oct 30, 2017

I have a problem with selection with your PR.
When I right click to show contextMenu at a point "P", when I discard contextMenu, xterm is on SelectionMode starting on this point "P" (but I haven't any mouse button pressed).
See:
contextmenu

@chabou

This comment has been minimized.

Collaborator

chabou commented Oct 30, 2017

Can you remove "Open Selection as URL" and "Search the webfor Selection"? I don't think it should be a core feature. But it is perfect to filter Copy/Cut in contextMenu depending on selection presence.

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Oct 30, 2017

@chabou I've removed the extended menu with the additional options. I noticed canary has the ability to click on URLs anyway, so maybe Open Selection as URL isn't so useful now

we can discuss creating plugins to accomplish those items once this lands/how to expand on this so plugin authors can modify the context menu

@Stanzilla

This comment has been minimized.

Collaborator

Stanzilla commented Oct 30, 2017

The CircleCI failure looks unrelated.

@rickycodes

This comment has been minimized.

Contributor

rickycodes commented Oct 30, 2017

@Stanzilla yea i noticed that, but it seems unrelated (the build was passing earlier and I haven't made any adjustments that would have changed that)

@chabou

This comment has been minimized.

Collaborator

chabou commented Nov 3, 2017

I can't reproduce #2001 (comment) anymore 🤔

Let's merge this. We'll fix bugs if needed: not a big deal.

@chabou

This comment has been minimized.

Collaborator

chabou commented Nov 3, 2017

@rickycodes I can't push to your branch. Did you allow maintainers to modify your branch?
I resolved conflicts (in GitHub interface) and Github doesn't see my last commit 😞.
Can you push a empty commit to trigger it ?

@rauchg

This comment has been minimized.

Contributor

rauchg commented Nov 3, 2017

This feature is 🔥

@chabou

chabou approved these changes Nov 3, 2017

@chabou chabou merged commit 62e29ef into zeit:canary Nov 3, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chabou

This comment has been minimized.

Collaborator

chabou commented Nov 3, 2017

Thank you so much for your help and your awesome work 🙏
Really sorry for the multiple delays along this PR.

@rickycodes rickycodes deleted the rickycodes:issue-265 branch Jan 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment