Skip to content
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

Use QString literals #16652

Merged
merged 2 commits into from
Mar 18, 2022
Merged

Use QString literals #16652

merged 2 commits into from
Mar 18, 2022

Conversation

Chocobo1
Copy link
Member

This patch covers src/gui and some leftovers from previous commit.

@Chocobo1 Chocobo1 added the Code cleanup Clean up the code while preserving the same outcome label Mar 15, 2022
@Chocobo1 Chocobo1 added this to the 4.5.0 milestone Mar 15, 2022
@Chocobo1 Chocobo1 force-pushed the literal branch 2 times, most recently from 300431e to ed29427 Compare March 15, 2022 08:26
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
glassez
glassez previously approved these changes Mar 16, 2022
Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope it doesn't contain any inappropriate changes. I can't check all this thoroughly.

@Chocobo1
Copy link
Member Author

@now-im @xavier2k6
Just to give you guys a heads-up. After merging this PR (which should happen very soon) you might experience some "merge conflicts" in PR #15769, sorry for the inconvenience.

The solution is simple, see below:

UIThemeManager::instance()->getIcon("document-properties");      // if the conflict is here
UIThemeManager::instance()->getIcon(u"document-properties"_qs);  // add the 'u' prefix and '_qs' postfix to fix it


UIThemeManager::instance()->getIcon(QLatin1String("document-properties"));  // here there should be no conflict so you don't have to do modify anything *for now*
// in the future it will be changed to the following as well
UIThemeManager::instance()->getIcon(u"document-properties"_qs); 

@Mazino-Urek
Copy link
Contributor

@Chocobo1 Got it. Thanks.

@xavier2k6
Copy link
Member

Just to give you guys a heads-up

Thank you, shouldn't be too much of a problem.

@now-im If you run in to problems re-basing etc. give me a shout. (I'm still currently not getting notification from mentions etc. but I should come across things when I'm looking through the Issues tickets/PR's)

@Mazino-Urek
Copy link
Contributor

@now-im If you run in to problems re-basing etc. give me a shout. (I'm still currently not getting notification from mentions etc. but I should come across things when I'm looking through the Issues tickets/PR's)

Ok.

This patch covers src/gui and some leftovers from previous commit.
@Chocobo1
Copy link
Member Author

Chocobo1 commented Mar 18, 2022

@glassez
Resolved merge conflicts and another small clean up.

@Chocobo1 Chocobo1 merged commit e71e97c into qbittorrent:master Mar 18, 2022
@Chocobo1 Chocobo1 deleted the literal branch March 18, 2022 06:09
@Mazino-Urek
Copy link
Contributor

@Chocobo1 @xavier2k6 Successfully rebased.

@@ -168,7 +168,7 @@ QVariant TransferListModel::headerData(int section, Qt::Orientation orientation,
{
switch (section)
{
case TR_QUEUE_POSITION: return QChar('#');
case TR_QUEUE_POSITION: return u'#';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This column is now shown as "35". Any char in place of # here returns a number as column name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, QVariant treats char16_t as numeric type so you still need to explicitly tell it that we want QChar or QString:
case TR_QUEUE_POSITION: return QChar(u'#'); or case TR_QUEUE_POSITION: return u'#'_qs;

Copy link
Member Author

@Chocobo1 Chocobo1 Mar 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will fix.

UPDATE: fixed in PR #16669.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code cleanup Clean up the code while preserving the same outcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants