Move trackset-features into library subfolder #2715

xeruf · 2020-04-25T21:48:57Z

Follow-up to my previous PR. The second commit is a pure refactor/reformat, no code changes. Tested with SCons & CMake.

xeruf · 2020-04-26T07:30:11Z

@Be-ing can't request review directly :)

There is the "Triage" role on GitHub now, which would give access to such things, are you using that?

uklotzde

Good idea to file a PR before actually changing any code.

Please check the formatting of each file, especially check for long lines. Sometimes you need need to give clang-format a hint to get it right.

src/library/trackset/crate/cratetablemodel.cpp

src/library/banshee/bansheeplaylistmodel.cpp

src/library/baseexternalplaylistmodel.cpp

src/library/hiddentablemodel.cpp

The trailing slashes prevent clang-format from wrapping that text into a big chunk, see: https://stackoverflow.com/a/34362422

src/library/trackset/baseplaylistfeature.h

src/library/trackset/crate/cratetablemodel.cpp

xeruf · 2020-05-19T10:39:06Z

I have rebased on master and presumably fixed all mentioned problems :)

src/library/trackmodel.h

src/library/trackset/crate/cratestorage.cpp

src/library/trackset/crate/cratetablemodel.cpp

xeruf · 2020-05-19T19:05:13Z

I would like to have this PR merged as there don't seem to any substantial issues left. Deferred tasks:

TrackModel Capabilities to enum: Move trackset-features into library subfolder #2715 (comment)

@Holzhaus please check if I resolved #2715 (comment) correctly

Holzhaus

Thanks, looks mostly good.

src/library/playlisttablemodel.cpp

src/library/trackset/crate/cratestorage.cpp

src/library/playlisttablemodel.cpp

src/library/librarytablemodel.cpp

.clang-format

src/library/hiddentablemodel.cpp

.clang-format

xeruf · 2020-05-21T12:20:18Z

@Holzhaus all should be fixed now - again, please check above (#2715 (comment)) if I did it correctly and resolve the comment

Holzhaus · 2020-05-22T09:42:39Z

src/library/trackset/crate/cratefeature.cpp

+    return QStringLiteral("%1 (%2) %3")
            .arg(


Does QStringLiteral actually work with .arg()? I remember I tried that once and it didn't work...

I can confirm that it does work and also remember that it didn't previously. Maybe switching to QStringBuilder did the trick?

The tests are all succeeding, and I hope there is at least some testing covering the library. I changed this in so many places that it would be shocking if it wouldn't work and the tests would still succeed.

Holzhaus · 2020-05-22T09:49:32Z

There are some remaining style issues.
I recommend to use the pre-commit hook, to avoid future style discussions.

I USE CLANG-FORMAT! ALL THE TIME!

Sorry, but I can't take this anymore. For days almost all comments have been about issues of clang-format that I'm supposed to manually correct. I have better things to do.
Please either merge my PR as is or refrain from any style comments until you have adjusted it to actually produce the style you want. I can also look into that, but so far my suggestions about config changes haven't been well received.

pre-commit != clang-format. Please use the pre-commit hook, not just clang-format.
These hooks actually run clang-format twice: The first time it runs clang-format, the second time runs clang-format again with slightly different settings and only on long lines.

I didn't have any issues with formatting when using pre-commit. If you're are actually using that and it still doesn't format code correctly that's a bug and needs to be fixed.

xeruf · 2020-05-22T10:04:07Z

pre-commit != clang-format. Please use the pre-commit hook, not just clang-format.
These hooks actually run clang-format twice: The first time it runs clang-format, the second time runs clang-format again with slightly different settings and only on long lines.

Thanks for the clarification! I assumed it would simply run clang-format.

Made that more clear on the wiki: https://www.mixxx.org/wiki/doku.php/coding_guidelines#pre-commit

xeruf · 2020-05-22T12:25:37Z

I ran pre-commit over src/library/library.cpp and the whole src/library folder using git ls-files -- "src/library" | xargs pre-commit run --files` - nothing changed...

And no, I'm not doing a rebase. I just tried and ended up with horrible conflicts.

uklotzde · 2020-05-22T19:58:02Z

No need to delay this PR further due to code style discussions. We have identified some shortcomings of clang-format for multi-line string literals that could be resolved later.

daschuer · 2020-05-23T02:40:05Z

I am sorry to stress this formatting issue that much.
But vertical scrolling during a review is a no go for me.

I think the PR got to this state, because you probably have messed around with clang format during development.
Luckily this reveals an issue with our CI clang-format test on Travis.
I have added a PR that aims to fix it.
#2816

Now you have three options to continue here:

git rebase -i upstream/master
and amend each commit with the pre-commit hook changes.

git pull https://github.com/daschuer/mixxx.git pre-commit-clang
pre-commit run --source upstream/master --origin HEAD
git commit -a
git commit -a

Merge
clang-format xeruf/mixxx#3

xeruf · 2020-05-23T10:50:11Z

@daschuer I am leaning towards option 3, but this WILL create conflicts with my follow-up PR that I've already started working on because this took so long, which I am not fond of either...

Also, I am seeing quite a few things in there that make the code less readable...

xeruf · 2020-05-23T11:04:19Z

It's not as if I explicitly created new long lines. All the lines that got longer where solely because of clang-format. For the lines I worked on by hand I always checked the length.

I think the hard-wrap should leave a little buffer (maybe at 100), because sometimes a line needs to be a little longer to stay readable.

xeruf#3 (review)
Here a few comments on wrongs of the formatter

xeruf · 2020-05-23T11:08:10Z

I think the PR got to this state, because you probably have messed around with clang format during development.

Nope, I didn't mess around. I have configured my IDE to automatically run clang-format on files I work on, if that doesn't produce the correct coding style without the pre-commit hook, it should be adjusted. There was only one option I tried, which was AlwaysBreakBeforeMultilineStrings: true and didn't make much of a difference.

daschuer · 2020-05-23T12:01:53Z

I am leaning towards option 3, but this WILL create conflicts with my follow-up PR that I've already started working on because this took so long, which I am not fond of either...

OK so go ahead. Since the clang-format commit does not change the code, the conflicts should be easy to solve.

xeruf · 2020-05-30T14:59:36Z

Alright, I hope we are finally done here 😅

Holzhaus · 2020-06-04T23:54:11Z

Some merge conflicts have emerged.

xeruf · 2020-06-05T07:41:32Z

Me: Creates PR
Community: Reviews PR
Me: Let's merge
Community: First, let's fix the code style
Me: Okay, but let's finally merge!
Github: A wild conflict has appeared!
😅

xeruf · 2020-06-05T07:50:10Z

Fixed, was merely about imports :)

xeruf · 2020-06-11T17:43:17Z

@Holzhaus please merge, before more things appear :)

daschuer

LGTM, Thank you. And sorry for all the hassle.

Holzhaus

LGTM

uklotzde reviewed Apr 26, 2020

View reviewed changes

src/library/trackset/crate/cratetablemodel.cpp Outdated Show resolved Hide resolved

xeruf force-pushed the refactor-basetrackset branch from dee23e9 to 4f840a3 Compare April 26, 2020 12:08

xeruf commented Apr 26, 2020

View reviewed changes

src/library/banshee/bansheeplaylistmodel.cpp Outdated Show resolved Hide resolved

src/library/baseexternalplaylistmodel.cpp Outdated Show resolved Hide resolved

src/library/hiddentablemodel.cpp Outdated Show resolved Hide resolved

xeruf requested a review from uklotzde April 26, 2020 12:11

uklotzde added the code quality label Apr 26, 2020

Use QStringLiteral and value binding for library queries

085af01

The trailing slashes prevent clang-format from wrapping that text into a big chunk, see: https://stackoverflow.com/a/34362422

xeruf force-pushed the refactor-basetrackset branch from 1d4905f to bbabfad Compare April 26, 2020 19:59

daschuer requested changes May 4, 2020

View reviewed changes

src/library/trackset/baseplaylistfeature.h Outdated Show resolved Hide resolved

src/library/trackset/crate/cratetablemodel.cpp Outdated Show resolved Hide resolved

daschuer added this to the 2.4.0 milestone May 4, 2020

xeruf added 4 commits May 19, 2020 11:25

Move childModel to BaseTrackSetFeature

29b3b20

Move all trackset-features into library subdirectory

e566fae

Clean up CapabilitiesFlags and apply clang format

d31825b

Clean up HiddenTableModel::setTableModel

e5861ed

xeruf force-pushed the refactor-basetrackset branch from 63bf05a to c1a6054 Compare May 19, 2020 10:37

xeruf requested review from Holzhaus and daschuer May 19, 2020 10:38

Holzhaus requested changes May 19, 2020

View reviewed changes

Fix formatting in CreateFeature classes

5477354

xeruf requested a review from Holzhaus May 19, 2020 19:05

Inline anonymous method in CrateFeature

f6e1eea

Holzhaus reviewed May 20, 2020

View reviewed changes

uklotzde suggested changes May 20, 2020

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

Reformat long lines & clean up tablemodels

bd4a67d

xeruf requested review from uklotzde and Holzhaus May 21, 2020 09:45

xeruf force-pushed the refactor-basetrackset branch from 694ee97 to bd4a67d Compare May 21, 2020 12:08

xeruf added 2 commits May 22, 2020 11:15

Embrace QStringLiteral in TrackSet features

76f5fae

Replace tr "[noun]" with "//:"

d40ade3

Holzhaus reviewed May 22, 2020

View reviewed changes

Fix comment spaces in src/library

9eb5ec6

uklotzde approved these changes May 22, 2020

View reviewed changes

daschuer and others added 2 commits May 27, 2020 11:56

clang-format

4f032f7

Adjust reformattings

f8d44a4

xeruf requested review from uklotzde, daschuer and Holzhaus May 30, 2020 14:59

Merge branch 'master' into refactor-basetrackset

eb756ee

Merge branch 'master' into refactor-basetrackset

8757571

daschuer approved these changes Jun 11, 2020

View reviewed changes

Holzhaus approved these changes Jun 11, 2020

View reviewed changes

Holzhaus merged commit 2ea349e into mixxxdj:master Jun 11, 2020

xeruf deleted the refactor-basetrackset branch June 12, 2020 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move trackset-features into library subfolder #2715

Move trackset-features into library subfolder #2715

xeruf commented Apr 25, 2020 •

edited

Loading

xeruf commented Apr 26, 2020

uklotzde left a comment

xeruf commented May 19, 2020

xeruf commented May 19, 2020 •

edited

Loading

Holzhaus left a comment

xeruf commented May 21, 2020

Holzhaus May 22, 2020

uklotzde May 22, 2020

xeruf May 22, 2020

Holzhaus commented May 22, 2020

xeruf commented May 22, 2020

xeruf commented May 22, 2020 •

edited

Loading

uklotzde commented May 22, 2020

daschuer commented May 23, 2020 •

edited

Loading

xeruf commented May 23, 2020

xeruf commented May 23, 2020

xeruf commented May 23, 2020

daschuer commented May 23, 2020

xeruf commented May 30, 2020

Holzhaus commented Jun 4, 2020

xeruf commented Jun 5, 2020

xeruf commented Jun 5, 2020

xeruf commented Jun 11, 2020

daschuer left a comment

Holzhaus left a comment

Move trackset-features into library subfolder #2715

Move trackset-features into library subfolder #2715

Conversation

xeruf commented Apr 25, 2020 • edited Loading

xeruf commented Apr 26, 2020

uklotzde left a comment

Choose a reason for hiding this comment

xeruf commented May 19, 2020

xeruf commented May 19, 2020 • edited Loading

Holzhaus left a comment

Choose a reason for hiding this comment

xeruf commented May 21, 2020

Holzhaus May 22, 2020

Choose a reason for hiding this comment

uklotzde May 22, 2020

Choose a reason for hiding this comment

xeruf May 22, 2020

Choose a reason for hiding this comment

Holzhaus commented May 22, 2020

xeruf commented May 22, 2020

xeruf commented May 22, 2020 • edited Loading

uklotzde commented May 22, 2020

daschuer commented May 23, 2020 • edited Loading

xeruf commented May 23, 2020

xeruf commented May 23, 2020

xeruf commented May 23, 2020

daschuer commented May 23, 2020

xeruf commented May 30, 2020

Holzhaus commented Jun 4, 2020

xeruf commented Jun 5, 2020

xeruf commented Jun 5, 2020

xeruf commented Jun 11, 2020

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

xeruf commented Apr 25, 2020 •

edited

Loading

xeruf commented May 19, 2020 •

edited

Loading

xeruf commented May 22, 2020 •

edited

Loading

daschuer commented May 23, 2020 •

edited

Loading