Refactor/pref sound tab order #11926
Conversation
|
the outdated compiler on macos is really annoying... lets update it #11927 |
|
The original PR was against 2.4. Was the switch to main desired? |
Yes and no, I forgot to switch away from the default |
|
It looks like only the |
| namespace { | ||
|
|
||
| bool soundItemAlreadyExists(const AudioPath& output, const QWidget& widget) { | ||
| for (const QObject* obj : widget.children()) { |
There was a problem hiding this comment.
We use p prefix for pointer and smart pointers.
There was a problem hiding this comment.
yup, forgot to change when copying from previous code
| bool soundItemAlreadyExists(const AudioPath& output, const QWidget& widget) { | ||
| for (const QObject* obj : widget.children()) { | ||
| auto item = qobject_cast<const DlgPrefSoundItem*>(obj); | ||
| if (!item || item->type() == output.getType()) { |
There was a problem hiding this comment.
I don't understand this condition. Is it inverted?
Please add a comment if it is correct.
There was a problem hiding this comment.
no, that was a bug thank you. fixed in upcoming commit
| } | ||
| AudioPathType type = output.getType(); | ||
| // TODO who owns this? | ||
| DlgPrefSoundItem* pSoundItem = AudioPath::isIndexed(type) |
There was a problem hiding this comment.
Can the conditional go to the last optional argument? The same below.
There was a problem hiding this comment.
Technically a little leaky as the caller now knows the default value, but who cares. done.
| const AudioPath& rhs) { | ||
| // Exclude m_channelGroup from comparison! | ||
| // See also: hashValue()/qHash() | ||
| // TODO: Why?? |
There was a problem hiding this comment.
Do you have an explanation here?
There was a problem hiding this comment.
no, it came from the previous code and I didn't bother much into it. likely because m_channelGroup is synthesized or not important for distinguishing instances of the class.
src/soundio/soundmanagerutil.h
Outdated
|
|
||
| // TODO: remove this ifdef when AppleClang ships a libc++ | ||
| // implementation with operator<=> for tuples (XCode 14 probably) | ||
| #ifdef __APPLE__ |
There was a problem hiding this comment.
Just use operator< for all. The rest of the space ship operator is not used anyway.
There was a problem hiding this comment.
Right, but its bad practice to only define the ordering partially and operator<=> is better than having to define all six ordering overloads manually (especially considering the friend boilerplate).
There was a problem hiding this comment.
CI passes on all platforms, do you still insist on the manual implementation?
There was a problem hiding this comment.
It fails also on Ubuntu Focal unfortunately. It knows nothing about <=>
Right, but its bad practice to only define the ordering partially
It is also bad practice to define unused functions.
So in this case I would just replace <=> by operator< for all and nothing else.
This is IMHO enough and does not clutters the code with #ifdefs for code that is not used anyway.
I will do a PR, for this because you are not able to test it.
You can than use it to remove the Apple conditions in a fixup.
There was a problem hiding this comment.
Here it is: Swiftb0y#17 tested and fully working.
Swiftb0y
force-pushed
the
refactor/pref-sound-tab-order
branch
from
ded4252
to
c468504
Compare
|
I believe I've addressed all your review requests. please rereview |
Swiftb0y
force-pushed
the
refactor/pref-sound-tab-order
branch
from
c468504
to
d27dc61
Compare
|
Thank you. |
|
Thank you. |
|
Working btw, thanks! |
|
Thank you for testing. I didn't test thoroughly tbh, good to know it works. |
Alternative to #11921 along with minor refactorings