modernize ControllerManager #13627
modernize ControllerManager #13627
Conversation
* move equality comparision closer to type definition. * remove trailing null-entry in support array * modernize allowlist search
* replace magic (unnamed) values with proper constants * use idiomatic array iteration
* explicitly initialize all members * use unique_ptr for owning pointers * use std::algorithm where applicable * use compile-time literals where possible
…ice" Because qt is too old... This reverts commit b021f64.
| @@ -120,10 +118,13 @@ QList<Controller*> HidEnumerator::queryDevices() { | |||
| continue; | |||
| } | |||
|
|
|||
| HidController* newDevice = new HidController(std::move(deviceInfo)); | |||
| m_devices.push_back(newDevice); | |||
| m_devices.push_back(std::make_unique<HidController>(std::move(deviceInfo))); | |||
There was a problem hiding this comment.
not that independent of my changes, this is currently broken when querying devices multiple times because we don't clear the list from the previous query... Not sure if I should fix that here because its not feasible to test nor do I have enough non-keyboard/-mice HID devices here.
There was a problem hiding this comment.
I think we should fix it anyway. Main is still in alpha stage.
There was a problem hiding this comment.
Looks good overall! One request tho, could you make sure that updated pointer variables are prefixed with p... to help with readability?
One mild concerned is around BULK, not directly related to your change: Interface also appears to be relevant on Linux, and not using them leads to dmesg warnings, so I'm wondering if would be worth removing the preprocessor around this. If you're keen to give it a try, I'm happy to test it with my devices to confirm it works as expected!
|
Will look into both. Thanks for taking a look @acolombier |
This fixes a (hypothetical) issue which could cause duplicate HID and Bulk devices to be created when querying for devices multiple times. This has not not happened yet because devices so far have only been queried once on startup.
Swiftb0y
force-pushed
the
refactor/modernize-controllermanager
branch
4 times, most recently
from
208692b
to
b8567ae
Compare
Swiftb0y
force-pushed
the
refactor/modernize-controllermanager
branch
4 times, most recently
from
4fca48b
to
2925eee
Compare
Swiftb0y
force-pushed
the
refactor/modernize-controllermanager
branch
2 times, most recently
from
4762ae4
to
840e4a5
Compare
| @@ -1,13 +1,15 @@ | |||
| #include "controllers/midi/portmidienumerator.h" | |||
|
|
|||
| #include <portmidi.h> | |||
| #include <qlatin1stringview.h> | |||
There was a problem hiding this comment.
What is this used for? Looks like it doesn't exist on MacOS
There was a problem hiding this comment.
whoops, yeah clangd tries its best to add the relevant includes but its gets it wrong and I don't notice it. I'll remove and force push (and look into disabling that for certain headers).
There was a problem hiding this comment.
fyi in case any1 else stumbles over this, see https://clangd.llvm.org/guides/include-cleaner#adjusting-behavior and use IgnoreHeaders: q.*\.h until QTBUG-124159 is fixed.
Swiftb0y
force-pushed
the
refactor/modernize-controllermanager
branch
from
840e4a5
to
3d5b851
Compare
Swiftb0y
force-pushed
the
refactor/modernize-controllermanager
branch
from
3d5b851
to
6b0c83f
Compare
And a little bit of the surrounding code. This is what I have so far, though the multithreading code could use a little larger restructuring. based on (outdated) #13622.