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

Feature: Add selected toolbar buttons to MacBook Pro Touch Bar #9511

Merged
merged 1 commit into from Sep 23, 2021
Merged

Feature: Add selected toolbar buttons to MacBook Pro Touch Bar #9511

merged 1 commit into from Sep 23, 2021

Conversation

ddebruijne
Copy link
Contributor

Motivation / Problem

I've wanted to implement some functionality on the MacBook Pro's touch bar, and noticed OpenTTD was not using it yet.
 
As this was mainly a personal coding exercise, I don't currently have my expectations set on having this change merged into the mainline branch, but I figured to send in a PR anyways in case the maintainers do want to have it integrated :)
 

Description

This implements selected in-game toolbar buttons on the MacBook Pro's Touch Bar, the included commit adds the following buttons:
• Pause
• Fast-Forward
• Zoom in/out
• Most building windows
 
With this change players could enjoy having these shortcuts on a familar place on the keyboard which is also used by the rest of the OS.
 
openttd_touchbar
 

Limitations

The player can't customize the touch bar yet. In this commit, it is fixed to instant actions or windows that can replace each other.
 

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
        * ai_changelog.hpp, gs_changelog.hpp need updating.
        * The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
        * newgrf_debug_data.h may need updating.
        * PR must be added to API tracker

@orudge
Copy link
Contributor

orudge commented Aug 23, 2021

The concept seems good to me, but unfortunately the graphics aren't working on my Mac:

touchbar

The buttons do do what they say they should do, however. :)

I suspect some tidying up of parts of the code would be necessary too.

@michicc
Copy link
Member

michicc commented Aug 23, 2021

It is a bit niche and we don't really have any properly active OSX devs, so the chances aren't that high.

Nevertheless, if it should be included, it would definitely need a pass through https://wiki.openttd.org/en/Development/Coding%20style

@ddebruijne
Copy link
Contributor Author

Thanks for taking a look!
Text appearing instead of the sprite is a fallback in place when it can't generate the button sprite. I'll have a look to see where that could go wrong and patch up the style issues in the process.

@ddebruijne
Copy link
Contributor Author

Looks like the checker didn't like that I made a separate commit - do they need to be squashed?

@orudge I've made a small tweak to the objective-c file which may fix it, but it's quite tough to test as I can't repro what you are seeing. I saw from your photo that you were generating a DMG which I couldn't get working on my machine due to signing issues I believe, maybe that could be related?

In any case, if this doesn't fix it, I'd rather close the PR and not integrate it. If the sprites don't work, it kinda defeats the point of the implementation in my opinion, and is not clean. 😄

@michicc
Copy link
Member

michicc commented Aug 24, 2021

We would indeed squash a cleanup commit, especially as the commit checker checks each commit individually:

Details: https://github.com/OpenTTD/OpenTTD/runs/3412939438?check_suite_focus=true

@orudge
Copy link
Contributor

orudge commented Aug 25, 2021

I'll test it when I get a chance (quite busy at the moment), and if it's still not working, might be able to do some debugging myself.

@TrueBrain TrueBrain added this to the 12.0 milestone Sep 12, 2021
@orudge
Copy link
Contributor

orudge commented Sep 13, 2021

OK, looks like this is now working for me. :)

@ddebruijne
Copy link
Contributor Author

Thanks for checking, appreciate it!
Squashed the commits and rebased to latest, so hopefully the tester should be pleased with it now too :)

src/gfx.cpp Outdated Show resolved Hide resolved
src/gfx.cpp Outdated Show resolved Hide resolved
src/gfx.cpp Outdated Show resolved Hide resolved
src/gfx.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

A very small nitpick. Doesn't need fixing, but if there is a need for rebase, might be nice to tackle this too.

Code-wise PR looks good to me. @michicc : do you have any additional comments?

src/gfx.cpp Outdated Show resolved Hide resolved
TrueBrain
TrueBrain previously approved these changes Sep 19, 2021
src/gfx.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain dismissed their stale review September 19, 2021 21:10

okay, let's tackle the two minor things before merge :)

Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

If we are into nitpicking, I have some nits, too. (Nothing in any way serious.)

src/gfx.cpp Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_wnd.mm Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_wnd.mm Outdated Show resolved Hide resolved
src/video/cocoa/cocoa_wnd.mm Outdated Show resolved Hide resolved
@ddebruijne
Copy link
Contributor Author

Thanks for the feedback! I'll try to address these points this week 👍🏻

@michicc michicc merged commit 753b1d7 into OpenTTD:master Sep 23, 2021
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.

None yet

6 participants