Added Windows support to systemvolume plugin
ClosedPublic

Authored by jambon on Nov 16 2018, 10:19 PM.

Details

Summary

I've added Windows support to the systemvolume plugin.

Test Plan

Move the volume sliders in the Android app

Diff Detail

Repository
R224 KDE Connect
Branch
windows-systemvolume
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5056
Build 5074: arc lint + arc unit
jambon created this revision.Nov 16 2018, 10:19 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptNov 16 2018, 10:19 PM
jambon requested review of this revision.Nov 16 2018, 10:19 PM
jambon updated this revision to Diff 45699.Nov 18 2018, 1:49 AM

Fixed some stuff

jambon updated this revision to Diff 45732.Nov 18 2018, 4:23 PM

More fixes

jambon updated this revision to Diff 45737.Nov 18 2018, 5:48 PM
  • Improved Error Checking
jambon retitled this revision from [RFC] Added Windows support to systemvolume plugin to Added Windows support to systemvolume plugin.Nov 18 2018, 10:15 PM
jambon edited the summary of this revision. (Show Details)
jambon edited the test plan for this revision. (Show Details)
jambon edited the summary of this revision. (Show Details)EditedNov 19 2018, 2:25 AM

I tested this for a little bit and it seems to work pretty well. I think this is ready to be merged.

albertvaka accepted this revision.Nov 19 2018, 12:09 PM
albertvaka added a subscriber: albertvaka.

I have a couple minor comments, but looks good to me. It would be nice to implement a really simple MPRIS plugin for Windows now (even if only sending multimedia key inputs), so this can be used form the Android app :P

plugins/systemvolume/systemvolumeplugin-win.cpp
261–270

Can we merge these two into a single packet? Checking the Android side, you will also need to change an "else if" to become an "if", but I think it's cleaner.

plugins/systemvolume/systemvolumeplugin-win.h
29

If these two are private classes, I prefer having them defined in the .ccp file instead of nested here.

This revision is now accepted and ready to land.Nov 19 2018, 12:09 PM
jambon updated this revision to Diff 45839.Nov 19 2018, 9:34 PM
jambon edited the summary of this revision. (Show Details)

Done.

jambon marked 2 inline comments as done.Nov 19 2018, 9:37 PM

Can you do the Android side? Since you have commit access you can just commit directly. I'm too lazy to clone the repo and create a diff for a one word change.

This revision was automatically updated to reflect the committed changes.