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

Add a setting to control the function of F14 / F15 keys #602

Merged
merged 8 commits into from
Sep 21, 2021

Conversation

vtns
Copy link
Contributor

@vtns vtns commented Sep 16, 2021

Hi
The request contains the change which add a preference / checkbox to control whether if F14 / 15 key acts as brightness control or not.

The request also contains minor fix to Japanese localized text.
Feel free to drop this since there are still many unlocalized text anyway.

@waydabber
Copy link
Member

waydabber commented Sep 16, 2021

Hi @koyama-tadayoshi, thanks for the contribution! The solution looks solid. The only issue I have that a lot of things were rewritten in 3.1.0 and even though I can commit this into master, it won't be compatible with the new code and merging would be tedious.

The setting could go to the Keyboard tab of 3.1.0 but probably there will be a new Shortcuts tab, where the keyboard shortcuts (including custom shortcuts which is an old request) will be managed in general.

Screen Shot 2021-09-16 at 21 04 35

Note that this screenshot is the advanced view, the normal view is much simpler:

Screen Shot 2021-09-16 at 21 09 48

The setting about F14/F15 would obviously be classified as an advanced option.

@vtns
Copy link
Contributor Author

vtns commented Sep 18, 2021

Wow
3.1.0 looks cooler. I'm looking forward to it.

including custom shortcuts which is an old request
When you provide the way to customize shortcut, there may be no need for this option (about F14 / F15).
since we can have MediaKeyTap.useAlternateBrightnessKeys always be set to false
and the user who want to control brightness with F14 / F15 still be able to do it using customize key feature.

Anyway Thanks lot for bringing the great tool.
I'm using your application everyday.

@waydabber waydabber added the Status: Conflicts Issue has conflicts that needs to be resolved before merging label Sep 18, 2021
@waydabber
Copy link
Member

Hi @vnts, I'll leave this here for now and marked it with Conflicts. I'll use your code to add the F14/F15 thing to 3.1.0 if it is ok with you.

@JoniVR - alternatively we might add this to 3.0.0 and maybe release this and the recent translations as 3.1.0 and the version I am working on in the current 3.1.0 branch could be reclassified to 4.0.0 as it is shaping to be a rather big release with probably even more things in it than 2.1.0->3.0.0. It will take time to properly finish and test it, also it will need some betas as well. What do you think?

@vtns
Copy link
Contributor Author

vtns commented Sep 18, 2021

I'll use your code to add the F14/F15 thing to 3.1.0 if it is ok with you.

Of course, it's ok. Sorry for my misunderstanding.

waydabber
waydabber previously approved these changes Sep 20, 2021
@waydabber waydabber removed the Status: Conflicts Issue has conflicts that needs to be resolved before merging label Sep 20, 2021
@waydabber
Copy link
Member

Hi @JoniVR, @the0neyouseek - what should be the way forward regarding this? I renamed the existing 3.1.0 branch to 4.0.0 as it is still in development. If you agree with the change made by @vtns then we could merge this to the master and release s 3.1.0 with this and the updated translations that were added during the past two weeks. What do you say?

@waydabber waydabber requested review from the0neyouseek and removed request for reitermarkus September 20, 2021 17:07
@JoniVR
Copy link
Member

JoniVR commented Sep 20, 2021

@waydabber Sounds good to me, Do we want to go ahead and release v3.1.0 in combination with 4.0.0 beta or do we wait with v4.0.0 betas for a bit?

JoniVR
JoniVR previously approved these changes Sep 20, 2021
Copy link
Member

@JoniVR JoniVR left a comment

Choose a reason for hiding this comment

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

One thing we should note with this change is that the default behaviour changes from these keys being enabled to being disabled, so we might get some issues regarding this.

We could either default the toggle to enabled or just leave it as is and deal with the issues. Though I doubt many people will notice.

@JoniVR
Copy link
Member

JoniVR commented Sep 20, 2021

Related to #100

@waydabber
Copy link
Member

waydabber commented Sep 20, 2021

@waydabber Sounds good to me, Do we want to go ahead and release v3.1.0 in combination with 4.0.0 beta or do we wait with v4.0.0 betas for a bit?

I am still working on it and also I did not test it properly so it might not be entirely stable yet. Now I am trying to do something different than last time - I'll implement whatever should be implemented and then use the betas to actually aim for stability instead of having a moving target. :)

@waydabber
Copy link
Member

One thing we should note with this change is that the default behaviour changes from these keys being enabled to being disabled, so we might get some issues regarding this.

We could either default the toggle to enabled or just leave it as is and deal with the issues. Though I doubt many people will notice.

Hmm you are right. It would have been better to add this as a false-default option that does not upset existing behavior (it could have been named disableAltBrightnessKeys or something as having the alternative keys working is the normal state of the app).

@JoniVR
Copy link
Member

JoniVR commented Sep 20, 2021

Alright, let me know whenever you feel like we're ready for beta, my recent weeks have been pretty busy, as will this week be, but I'll make time for some releases when necessary. I was also still working on the slider but that's going pretty slowly..

@JoniVR
Copy link
Member

JoniVR commented Sep 20, 2021

About the default, I honestly don't think it matters that much, 97% of users probably don't even have a keyboard with f14/15 keys, and the other 3% probably doesn't even know/care about it doubling as brightness keys.

@waydabber
Copy link
Member

Probably you are right. I don't know how many PC/generic keyboard users there are. I wasn't avare that F14/F15 maps to ScrLK and Pause until I tried to use MonitorControl on the Hackintosh I had to install to debug the Intel stuff and I stumbled upon it by accident even then.

@JoniVR
Copy link
Member

JoniVR commented Sep 20, 2021

Yeah, I had added the option in MediaKeyTap a while back but didn't get to implementing it in MonitorControl yet. It has been a long standing issue.

@JoniVR JoniVR dismissed stale reviews from waydabber and themself via ebca454 September 21, 2021 17:16
JoniVR
JoniVR previously approved these changes Sep 21, 2021
@JoniVR JoniVR changed the title Add a setting to control the function of F14 / F15 keys. Add a setting to control the function of F14 / F15 keys Sep 21, 2021
@JoniVR JoniVR merged commit b493a42 into MonitorControl:master Sep 21, 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

5 participants