-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add multi-sampling settings for QML #12546
Conversation
f5924e3
to
d204e9a
Compare
@Swiftb0y are you happy with the current solution? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm otherwise.
d204e9a
to
2e367bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more thorough review
2e367bd
to
d829aa8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you
friendly ping @acolombier ;) |
d829aa8
to
424385b
Compare
Sorry, I missed the notification on that PR @Swiftb0y ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
// Multi-Sampling | ||
#ifdef MIXXX_USE_QML | ||
if (CmdlineArgs::Instance().isQml()) { | ||
mulitSamplingComboBox->clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn... why did CI not catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codespell uses a dictionary of common misspellings and does not support camelcase at the moment. The chance that "mulitsamplingcombobox" is in the dict is rather small :)
const QString kScaleFactorKey = QStringLiteral("ScaleFactor"); | ||
const QString kStartInFullscreenKey = QStringLiteral("StartInFullscreen"); | ||
const QString kSchemeKey = QStringLiteral("Scheme"); | ||
const QString kResizableSkinKey = QStringLiteral("ResizableSkin"); | ||
const QString kLocaleKey = QStringLiteral("Locale"); | ||
const QString kTooltipsKey = QStringLiteral("Tooltips"); | ||
const QString kMultiSamplingKey = QStringLiteral("multi_sampling"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we don't call it "multi_sampling" instead of "msaa" everywhere? The latter is much less ambiguous IMHO.
sorry for the review race-condition... |
my bad, my review came too late xD |
This extends the work made on #12541 to address #12536 with a customizable multi-sampling level.