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

Conversation

@danskidb
Copy link
Contributor

@danskidb danskidb commented Aug 23, 2021

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 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 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

@danskidb
Copy link
Contributor Author

@danskidb danskidb commented Aug 23, 2021

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.

@danskidb
Copy link
Contributor Author

@danskidb danskidb commented Aug 24, 2021

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 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 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 orudge commented Sep 13, 2021

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

@danskidb danskidb force-pushed the feature/touchbar branch from b6b6879 to 2aa856e Sep 13, 2021
@danskidb
Copy link
Contributor Author

@danskidb danskidb commented Sep 13, 2021

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
@danskidb danskidb force-pushed the feature/touchbar branch from 2aa856e to ac6a2f7 Sep 15, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

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
src/gfx.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain dismissed their stale review Sep 19, 2021

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

Copy link
Member

@michicc michicc left a comment

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
@danskidb
Copy link
Contributor Author

@danskidb danskidb commented Sep 19, 2021

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

@danskidb danskidb force-pushed the feature/touchbar branch 3 times, most recently from e5254a5 to 6d4d1e1 Sep 21, 2021
@danskidb danskidb force-pushed the feature/touchbar branch from 6d4d1e1 to bed0595 Sep 21, 2021
@michicc michicc merged commit 753b1d7 into OpenTTD:master Sep 23, 2021
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants