Conversation
psieg
reviewed
Dec 2, 2020
| int count = QGuiApplication::screens().count(); | ||
|
|
||
| result = QString("%1%2\r\n").arg(CmdResultCountMonitor).arg(count); | ||
| result = QStringLiteral("%1%2\r\n").arg(CmdResultCountMonitor).arg(count); |
Owner
There was a problem hiding this comment.
When did you swtich to .arg(x,y) and when stick to .arg(X).arg(Y) ?
Author
There was a problem hiding this comment.
(QString1, QString2...) (up to 9 i think)
arg(type1).arg(type2)... which is probably equal to (QString(type1), QString(type2)...) allocation wise
| virtual ~LedDeviceAlienFx(); | ||
| QString name() const { return QStringLiteral("lightfx"); } | ||
| int maxLedsCount(); | ||
| int defaultLedsCount() { return 1; } |
Owner
There was a problem hiding this comment.
just curious, why is the maxLedsCounts in the code file but the defaultLedsCount in the header?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I ran the build through clazy and it highlighted a lot of small things (still does) to improve
here are most of those things:
QStringLiteral(compile timeQStringbasically)also apparently doing this through
FLAGSdoes not work well becauseqmakewould still append whatever the default is frommkspec, so changing this viaCONFIGsolves thatconstsstd::chrono_literalsforQTimersQList::reserve()where possibleAfter that I tried building with Qt6 RC, and after some minor massaging it worked
QSettingsassumes utf-8 now, so no need forsetIniCodec()(which is dropped from core anyway)QRegExp*isQRegularExpression*nowQListworks slightly differently now and does not like forward declarations for item typesI'm not sure if there's more to be done for this part given what they say (like is it leaking as is or...)
but just by itself this PR has like 95% of the above (so just this would not build with Qt6 for ex)
once all 3 are merged, another quick pass would be needed (this one should probably go first)
while dealing with slots and signals I did some device refactoring, mostly de-copypasta'd UDP, and fixed couple of small things on the way
there's a lot more to do there, but that'll wait another time
and finally, removed remaining
QDekstopWidgetusage which I started doing a while ago with some deprecations, but at the time they were saying "use these other classes/methods" which themselves usedQDekstopWidgetbut were not flagged as deprecated for some reason and now everything gets removed in Qt6...these changes seem to work, but I can't test multiple displays
otherwise this builds and runs on all 3 platforms