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

Load a basic menu on macOS #26

Closed
wants to merge 1 commit into from
Closed

Conversation

fanzeyi
Copy link
Contributor

@fanzeyi fanzeyi commented May 5, 2019

This will allow copying texts and system shortcuts on macOS. (Pasting text doesn't work right now nvm it's some bug in oh-my-zsh.)

For example, ⌘W will be able to close current terminal window, ⌘M will be able to hide the current window.

image

image

@fanzeyi
Copy link
Contributor Author

fanzeyi commented May 5, 2019

Looking at the giant xib file now I'm thinking maybe I should create the menus programmatically.. Although that would mean translating a few hundred lines of Swift into Rust calling Objective-C. I'm not sure which solution is better now :/

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for this!

In your screen shot it looks like there are mac tabs in the terminal, which made me excited to think that you'd found a way to magically draw native mac tabs in the gui, but when I ran it, I couldn't use CMD-T to create a new tab :-(

Regarding the size of the nib/xib, I don't think it is terrible if we take out the menu items that aren't connected to working actions; that should reduce the size and make it not feel overly bloated.

One thing we don't have a really good story around at the moment in wezterm is allowing the user to define key bindings; they're hard coded right now. I'd like to make those configurable via the wezterm.toml, and I'd like to have a good answer around how we reconcile conflicts between the user's key bindings in the config and the settings in the nib file. I found that the nib overrides the hard coded key bindings before they make it to the window at the moment; it would be good to resolve those conflicts before we land this PR, but I would like to also have a way for the bindings in the code to take precedence to those in the nib file, so that when we do eventually add the code for user key bindings, they take precedence too. I think that might mean that we will eventually need to programmatically generate the menu.

Thinking a bit more about menus: on Windows and Linux we don't have the same menubar concept, and I don't want to add a menu bar in the xterm UI itself, but I have been thinking that a right-click context menu would be reasonable. If we do end up with a programmatically generated menu then it would be nice to eventually have that code do something to create a context menu on those other platforms. That's out of scope for this PR, but I wanted to give you an idea of where we might end up.

What do we need to do to get this PR landed? I think trimming the contents of the NIB would be good (see inline comments), as well as hooking up the NIB compilation step to CI.

If there's an easy way to allow the wezterm key bindings to take precedence that would be good to know, but I think just resolving the conflicting key assignments is good enough to start with.

rm -rf "$APP"
cp -r assets/macos/WezTerm.app "$APP" && \
cp target/release/wezterm "$APP" && \
ibtool --compile "$BASE_LPROJ/menu.nib" "$BASE_LPROJ/menu.xib" && \
Copy link
Owner

Choose a reason for hiding this comment

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

I had to run sudo xcode-select -s /Applications/Xcode_10.2.app to get ibtool to run; it doesn't run with the CLI only flavor of the developer tools. I think that is probably fine, but its worth checking that it will run in the ci/deploy.sh script which packages up the .zip file that is produced when I push a tag.

I think we need to add this same step to that script, and should probably change the .travis.yml to always run ci/deploy.sh in the script section, rather than in the before_deploy section so that it gets tested in PRs too.

</items>
</menu>
</menuItem>
<menuItem title="Format" id="jxT-CU-nIS">
Copy link
Owner

Choose a reason for hiding this comment

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

I think this menuItem should be removed; it's not applicable to the terminal and it blocks some of the existing hotkeys eg: CMD-T should create a new tab, but instead pops up a font dialog that isn't connected to anything

</items>
</menu>
</menuItem>
<menuItem title="Edit" id="5QF-Oa-p0T">
Copy link
Owner

Choose a reason for hiding this comment

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

I think most the sub items in this menu don't apply and can be removed

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

(putting this back in your queue)

wez added a commit that referenced this pull request Jun 8, 2019
Added CMD-W as a default binding for this action.

refs: #26
@wez
Copy link
Owner

wez commented Nov 4, 2019

a lot has changed internally since this PR was made. The idea I have for this in the future is to introduce a layer in the code that abstracts the concept of a window menu bar so that we can populate it for both mac and windows (linux is notably lacking a standard for this, and will require more effort to eventually implement).

I'm going to close this PR because it won't land with its current approach!

@wez wez closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants