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

implement AdvancedPrefsViewController with custom polling #97

Merged
merged 22 commits into from
Aug 28, 2019

Conversation

JoniVR
Copy link
Member

@JoniVR JoniVR commented May 18, 2019

partly implements changes discussed in #37.

This is what the menu currently looks like:
ezgif com-video-to-gif

I'm thinking we'd probably want monitor specific polling settings though?
I'd like some input on how that should look, should we use another NSPopupMenu with the friendlyName?

TODO:

  • Translations? (can be done through new PR's)
  • Make it work for each display specific (this is a bit hard to test for me since I only have one display, and I wanted to hear the feedback first)
  • Add longerDelay option
  • Add hideOsd option

Please feel free to add/make changes or point out things that still need changing, this is very much a work in progress.

@JoniVR JoniVR added the Type: Enhancement Issue is an app enhancement label May 18, 2019
@the0neyouseek
Copy link
Member

the0neyouseek commented Jun 5, 2019

While we're at it, here are some things I'd also like to add to advanced prefs :

  • Inputs for the max/min value of brightness/volume for sliders and using keyboard (with default 0 and 100 or max value from ddc)
  • Input to change the modifiers for smaller increment (with default alt + shift)
  • Reset all prefs button

@JoniVR
Copy link
Member Author

JoniVR commented Jun 5, 2019

Sounds like a good idea! I'm currently pretty busy studying but after I finish (which should be somewhere next week) I can continue to work on it.

Quick question, in the advanced prefs for polling, should I use a tableview for displaying the monitors and their settings like we do in DisplayPrefsController or just the way I did it in the screencapture up top but add an extra NSPopup button for each monitor? I have the NSPopup almost implemented but it might be better to use a CollectionView instead?

@the0neyouseek
Copy link
Member

Hi @JoniVR and good luck with your finals !

Let's try with an NSPopup for now as to not overload this view too much

@JoniVR
Copy link
Member Author

JoniVR commented Jun 13, 2019

Something else we could add here would be another NSPopup for trying out different replyDelays to make debugging easier for people experiencing issues.

JoniVR added 3 commits June 29, 2019 09:29
- added `DisplayManager` to manage display list
- replaced popup menu with tableview (just works better)
- added more specific cells
- added DisplayDelegate to notify of changes
TODO: find a way to refresh menu states after reset
@JoniVR
Copy link
Member Author

JoniVR commented Jul 2, 2019

Some notes:

  • I went with the tableview approach anyways because it's much cleaner to do in code, especially when trying to update data. It also looks better when there's no connected displays.

  • I've added the reset prefs button but we still need a way to refresh the menu states when the prefs are reset.

  • I've separated the logic for holding the displays array.
    Since we're using it in several places, it seemed like a better idea to me to move it to a separate DisplayManager. I was thinking about doing this using a singleton but eventually went with a dependency-injection like approach (if that's what you can call it). I'm not sure if you'll agree with this approach or not but let me know.

  • Adding the specificPollingMode to the whitelist still seems pretty impossible with the current implementation, unless I'm missing something. I think we could just get rid of it once we have the advanced settings implemented tbh.

updated gif

JoniVR added 9 commits July 2, 2019 23:12
Conflicts:
	MonitorControl.xcodeproj/project.pbxproj
	MonitorControl/AppDelegate.swift
	MonitorControl/Display.swift
	MonitorControl/Info.plist
	MonitorControl/View Controllers/DisplayPrefsViewController.swift
	MonitorControlHelper/Info.plist
In some cases (for example when holding down the volume buttons) the sliders would be changed on the background thread instead of the main thread
TODO: fix pollingMode not resetting
JoniVR added 3 commits July 9, 2019 20:57
@JoniVR
Copy link
Member Author

JoniVR commented Jul 9, 2019

I think we could start wrapping up this PR (translations and cleanup and such) and perhaps if it's ready, release it and then open an issue with other stuff we might want to add to the advanced prefs as to not make this PR too big and get the most important stuff out there for specific issue fixes. (#37, #105 for example)

current-state-gif

JoniVR added 3 commits July 9, 2019 23:28
using the DisplayDelegate was unreliable and only worked for one VC (obviously)
@JoniVR
Copy link
Member Author

JoniVR commented Jul 10, 2019

I'm thinking it might be a good idea to add a help button which links to a wiki page on this repo where some of these advanced settings could be explained:

image
@the0neyouseek, Would you be open to enabling the wiki so that I can add some stuff like Polling Count, Longer Delay and Hide OSD to it?

It would also be possible to do this with the Apple Help Book functionality but it might be better to have it on the repo?

@JoniVR JoniVR marked this pull request as ready for review August 15, 2019 19:25
@JoniVR
Copy link
Member Author

JoniVR commented Aug 15, 2019

Wiki page has been created:
https://github.com/the0neyouseek/MonitorControl/wiki/Advanced-Preferences

All we need now is testing/review and translations.. I'm not really sure if having so many translations is going to be a good idea long term though.. given that now we'd have to ask everyone to translate this.

I'm also still having build issues, I don't know if I'm the only one but I'm unable to solve them. Replacing the $(CURRENT_BUILD_NUMBER) with actual number inside of the Plist files seems to be a temp workaround for me..

@the0neyouseek
Copy link
Member

Hi @JoniVR I'll review the PR this week-end 😉

Copy link
Member

@the0neyouseek the0neyouseek left a comment

Choose a reason for hiding this comment

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

I've made some comments if you can check them out, otherwise great PR 👍

MonitorControl/UI/Cells/LongerDelayCellView.swift Outdated Show resolved Hide resolved
MonitorControl/AppDelegate.swift Show resolved Hide resolved
MonitorControl/Display.swift Outdated Show resolved Hide resolved
@the0neyouseek the0neyouseek added the Priority: Major Issue is major (e.g. A lot of things broken…) label Aug 27, 2019
@JoniVR JoniVR removed the request for review from reitermarkus August 27, 2019 13:06
@the0neyouseek
Copy link
Member

Looks good to me 👍

@JoniVR JoniVR merged commit dd3d026 into master Aug 28, 2019
@JoniVR JoniVR deleted the advanced-settings branch August 28, 2019 10:17
@stereonom
Copy link

Is there a way to download the new version anywhere with this new feature in it?

@JoniVR
Copy link
Member Author

JoniVR commented Sep 15, 2019

@stereonom Version 1.7.0 has been released which includes this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Major Issue is major (e.g. A lot of things broken…) Type: Enhancement Issue is an app enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants