Add vnc dimensions #657
Add vnc dimensions #657
Conversation
…'s `geometry` property
…n starting VNC sessions
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
============================================
+ Coverage 32.22% 32.51% +0.28%
Complexity 284 284
============================================
Files 39 39
Lines 2470 2488 +18
Branches 443 447 +4
============================================
+ Hits 796 809 +13
- Misses 1599 1604 +5
Partials 75 75
Continue to review full report at Codecov.
|
|
|
||
| private fun saveDeviceDimensions(sesson: Session) { | ||
| val deviceDimensions = DeviceDimensionsUtility() | ||
| deviceDimensions.saveDeviceDimensions(applicationContext) |
There was a problem hiding this comment.
Rather than injecting the applicationContext, I think it would be better to inject a WindowManager directly. That way, we can avoid passing around the Context god object.
There was a problem hiding this comment.
This makes mocking easier as well.
There was a problem hiding this comment.
When I added tests, I found that injecting WindowManager and DisplayMetrics made it easily testable
| height = displayMetrics.heightPixels | ||
| } | ||
|
|
||
| fun getLongerDimension(): Int { |
There was a problem hiding this comment.
I think it makes more sense to have getLongerDimension and getShorterDimension as private functions, and expose a function like getGeometry, the return value of which can be assigned directly to a session. That way more of the logic is encapsulated in this helper class.
There was a problem hiding this comment.
You could combine the theoretical getGeometry with the existing saveDeviceDimensions function to write this in a more functional way and avoid statefulness in this class.
| @@ -398,6 +400,33 @@ class AcraWrapper { | |||
| } | |||
| } | |||
|
|
|||
| class DeviceDimensionsUtility { | |||
There was a problem hiding this comment.
As an aside, I'd kind of like to avoid Utility in the name of classes as much as possible moving forward. I definitely set a precedent to do so and kind of regret it now. Something like DeviceDimensionsRetriever or even just DeviceDimensions.
There was a problem hiding this comment.
I changed this class name to just DeviceDimensions
| private var width = 720 | ||
| private var height = 1480 | ||
|
|
||
| fun saveDeviceDimensions(context: Context) { |
There was a problem hiding this comment.
I think this function is a bit of a misnomer, as it doesn't actually 'save' the device dimensions.
There was a problem hiding this comment.
I changed it to getDeviceDimensions
* Add DeviceDimensionsUtility to save and get device dimensions * When starting VNC session, save device dimensions and set the session's `geometry` property * Pass the session's `geometry` as an environment variable * Add `geometry` field to Session to hold screen dimensions. Useful when starting VNC sessions * Lint * Fix unit tests * Rename saveDeviceDImensions to getDeviceDimensions and add unit test * Add missing import
Describe the pull request
This PR introduces the ability for VNC to start with the resolution that closely matches the screen resolution of the device.
This will result in a VNC session that maximizes the available screen space.
Future work will include scaling the visuals so that font is more legible.
Link to relevant issues