Merge master to library-redesign by gramanas · Pull Request #1474 · mixxxdj/mixxx

gramanas · 2018-01-06T18:53:59Z

Hello,

This is a PR for merging master to jmigual-library-redesign.
I go through the merge requests and resolve the conflicts one by one, commiting every time there is one.

gramanas · 2018-01-06T22:02:23Z

The next conflict comes from the merge with this hash:
dccf88d

I am not able to correctly merge the LateNight changes.

Be-ing · 2018-01-06T22:06:56Z

I think the easiest approach would be to chose the conflicting files from #1357 then modify the skin file for the library to use the new widgets

poelzi · 2018-01-07T08:25:29Z

thank you @gramanas for doing this. I really hope we can then land this soonish

gramanas · 2018-01-07T14:44:37Z

Ok, it's up to date with the current master branch.

All the tests pass and everything seems to work as it should.

All default skins work fine, except Tango. It needs buttons for each lib feature and fixing issues with the 2 sidebars on the left + the album cover.

Be-ing · 2018-01-07T15:27:15Z

Build fails.
Windows:

[CXX] src\library\features\analysis\analysisfeature.cpp
analysisfeature.cpp
C:\projects\mixxx\src\library/dlgtagfetcher.h(21): error C2059: syntax error: 'constant'
C:\projects\mixxx\src\library/dlgtagfetcher.h(24): error C2143: syntax error: missing ';' before '}'
C:\projects\mixxx\src\library/dlgtagfetcher.h(24): error C2238: unexpected token(s) preceding ';'
C:\projects\mixxx\src\library/dlgtagfetcher.h(26): error C2059: syntax error: 'public'
C:\projects\mixxx\src\library/dlgtagfetcher.h(30): error C2059: syntax error: 'protected'
C:\projects\mixxx\src\library/dlgtagfetcher.h(34): error C2059: syntax error: 'private'
C:\projects\mixxx\src\library/dlgtagfetcher.h(42): error C2059: syntax error: 'private'
C:\projects\mixxx\src\library/dlgtagfetcher.h(44): error C2270: 'addDivider': modifiers not allowed on nonmember functions
C:\projects\mixxx\src\library/dlgtagfetcher.h(46): error C2270: 'addTrack': modifiers not allowed on nonmember functions
C:\projects\mixxx\src\library/dlgtagfetcher.h(59): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\projects\mixxx\src\library/dlgtagfetcher.h(59): error C2146: syntax error: missing ';' before identifier 'm_networkError'
C:\projects\mixxx\src\library/dlgtagfetcher.h(60): error C2059: syntax error: '}'
C:\projects\mixxx\src\library/dlgtagfetcher.h(60): error C2143: syntax error: missing ';' before '}'
scons: *** [win64_build\library\features\analysis\analysisfeature.obj] Error 2
scons: building terminated because of errors.
==============================
Building Mixxx failed.

macOS:

In file included from src/dialog/savedqueries/dlgsavedquerieseditor.cpp:4:
In file included from src/dialog/savedqueries/savedqueriestablemodel.h:6:
In file included from src/library/libraryfeature.h:16:
src/util/parented_ptr.h:26:15: error: invalid operands to binary expression ('parented_ptr<SavedQueriesTableModel>' and 'nullptr_t')
        if (u != nullptr) {
            ~ ^  ~~~~~~~
src/dialog/savedqueries/dlgsavedquerieseditor.cpp:15:23: note: in instantiation of function template specialization 'parented_ptr<SavedQueriesTableModel>::parented_ptr<SavedQueriesTableModel>' requested here
    auto pSaveModel = make_parented<SavedQueriesTableModel>(m_pFeature, 
                      ^
1 error generated.
scons: *** [osx64_build/dialog/savedqueries/dlgsavedquerieseditor.o] Error 1
scons: building terminated because of errors.

gramanas · 2018-01-07T15:41:09Z

@Be-ing maybe that's an error in appveyor.

I can't reproduce it locally (linux) and the files mentioned don't seem to have a any problem.

sblaisot · 2018-01-07T16:14:55Z

probably not an appveyor issue as you have the samme issue with 32 ans 64 bits build. more probably a code that is not portable enough or use features unsupported by msvs compiler.

gramanas · 2018-01-07T17:07:27Z

@sblaisot How can I solve those issues? I don't have access to OSX or Windows

daschuer · 2018-01-07T21:29:58Z

I am currently building this on my Linux machine. Review works quite well using gitk. GitHub fails here.

daschuer · 2018-01-07T21:49:02Z

C:\projects\mixxx\src\library/dlgtagfetcher.h(21): error C2059: syntax error: 'constant'

I assume that NOERROR is alread defined somewhere in a windows header. Please try to use an other name for this. The enum could also become a enum class to avoid issues the other way round.

daschuer · 2018-01-07T22:11:56Z

src/util/parented_ptr.h:26:15: error: invalid operands to binary expression ('parented_ptr' and 'nullptr_t')

this is probably because this
inline bool operator!= (const parented_ptr& p, std::nullptr_t) {
follows after the first use. You can try use

if (!u) {
or
if (!u.get()) {

daschuer · 2018-01-07T22:28:39Z

I have found nothing suspicious in the merge commits, so I think we can merge this after CI fixed is ready.
Thank you for the work.

gramanas · 2018-01-08T13:33:38Z

src/util/parented_ptr.h:26:15: error: invalid operands to binary expression ('parented_ptr' and 'nullptr_t')

this is probably because this
inline bool operator!= (const parented_ptr& p, std::nullptr_t) {
follows after the first use.

How can it compile with no problem in my machine but error in the bot's build?

gramanas · 2018-01-08T15:01:11Z

Ok, the mac one passes, but the windows still fails

daschuer · 2018-01-08T16:01:16Z

[CXX] src\library\features\analysis\analysisfeature.cpp
analysisfeature.cpp
C:\projects\mixxx\src\library/dlgtagfetcher.h(21): error C2059: syntax error: 'constant'
C:\projects\mixxx\src\library/dlgtagfetcher.h(24): error C2143: syntax error: missing ';' before '}'
C:\projects\mixxx\src\library/dlgtagfetcher.h(24): error C2238: unexpected token(s) preceding ';'
C:\projects\mixxx\src\library/dlgtagfetcher.h(26): error C2059: syntax error: 'public'
C:\projects\mixxx\src\library/dlgtagfetcher.h(30): error C2059: syntax error: 'protected'
C:\projects\mixxx\src\library/dlgtagfetcher.h(34): error C2059: syntax error: 'private'
C:\projects\mixxx\src\library/dlgtagfetcher.h(42): error C2059: syntax error: 'private'
C:\projects\mixxx\src\library/dlgtagfetcher.h(44): error C2270: 'addDivider': modifiers not allowed on nonmember functions
C:\projects\mixxx\src\library/dlgtagfetcher.h(46): error C2270: 'addTrack': modifiers not allowed on nonmember functions
C:\projects\mixxx\src\library/dlgtagfetcher.h(59): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\projects\mixxx\src\library/dlgtagfetcher.h(59): error C2146: syntax error: missing ';' before identifier 'm_networkError'
C:\projects\mixxx\src\library/dlgtagfetcher.h(60): error C2059: syntax error: '}'
C:\projects\mixxx\src\library/dlgtagfetcher.h(60): error C2143: syntax error: missing ';' before '}'
scons: *** [win32_build\library\features\analysis\analysisfeature.obj] Error 2
scons: building terminated because of errors.

daschuer · 2018-01-08T16:06:15Z

Mmm strange. You may try to prefix all enum members with MIXXX, just to be sure.
I have unfortunately no other idea. A really ugly hack could be to remove the enum at all an use integer numbers instead, just to try if we are on the right track.

daschuer · 2018-01-09T14:41:52Z

NO_ERROR And NOERROR is defined in winerror.h
You need to pick an other enum name.

Be-ing · 2018-01-09T16:17:55Z

Changing the old fashioned C enum to an enum class may work too.

gramanas · 2018-01-09T16:45:00Z

Now the error is different.

src\library\features\tracks\tracksfeature.cpp(32): error C2440: 'initializing': cannot convert from 'initializer list' to 'QStringList'
src\library\features\tracks\tracksfeature.cpp(32): note: No constructor could take the source type, or constructor overload resolution was ambiguous
src\library\features\tracks\tracksfeature.cpp(40): error C2440: 'initializing': cannot convert from 'initializer list' to 'QList<QStringList>'
src\library\features\tracks\tracksfeature.cpp(40): note: No constructor could take the source type, or constructor overload resolution was ambiguous

https://forum.qt.io/topic/43778/error-when-initializing-qstringlist-using-initializer-list/11

What about Q_COMPILER_INITIALIZER_LISTS, could it be what we are missing?

daschuer · 2018-01-09T21:20:24Z

I think I can remember a similar issue and that this is a compiler issue.
The Q_COMPILER_INITIALIZER_LISTS macro tells the Qt source if the compiler supports these lists or not. If I understand that right VC 2015 should support it but QT is not aware of it. So you can try to
#define Q_COMPILER_INITIALIZER_LISTS
at the very top of tracksfeature.cpp.

gramanas · 2018-01-09T22:30:30Z

Seems to be working!

Be-ing · 2018-01-11T00:53:23Z

Travis failed on Ubuntu with a curious error:

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

gramanas · 2018-01-11T01:00:21Z

I think that's because the compile messages and the tests are pretty verbose so it's more that 4MB, if you check the log you'll see everything is fine!

I just copied and pasted the raw log to vim, and after waiting for a few minutes I managed to save it:

$ du -h raw_log
4.3M    raw_log

Be-ing · 2018-01-11T01:11:38Z

It looks like a lot of the log spam is from the library tests recreating a fresh database for each test, which is addressed in #1462.

Be-ing · 2018-01-11T04:25:03Z

I'm testing this now and it is working quite well. I have noticed it takes a long time to switch to the Tracks feature.

Be-ing · 2018-01-11T04:38:00Z

This crashed after a few hours of use with AutoDJ. I was not running Mixxx in GDB so I don't have a backtrace, but I'll be testing this with GDB from now on. I was not interacting with Mixxx at the time of the crash; I only had AutoDJ going with the window minimized. Here is the end of the log:

Debug [Main]: WTrackTableView::loadTrackModel() LibraryTableModel(0x22c3820) 
Debug [Main]: LibraryTableModel(0x22c3820) select() took 86 ms 12 
Debug [Main]: LibraryTableModel(0x22c3820) select() took 84 ms 12 
Debug [Main]: switchToFeature 0x371b4f0 
Debug [Main]: WSearchLineEdit::restoreSearch( "" ) 
Debug [Thread (pooled)]: SoundSourceProxy - SoundSourceProvider "Xiph.org libFLAC" created a SoundSource for file "file:///home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 01 Dieuf.flac" of type "flac" 
Debug [Thread (pooled)]: MetadataSourceTagLib - Importing cover art from file "/home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 01 Dieuf.flac" with type 2 
Debug [Thread (pooled)]: TagLib - VorbisComment picture list is empty 
Debug [Thread (pooled)]: TagLib - No cover art found in VorbisComment tag 
Debug [Main]: "/home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 09 Vision.flac" fromTrackDuration = 56.1867 
Debug [Main]: "/home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 08 Salaam.flac" toTrackDuration =  534.773 
Debug [Main]: m_fadeDuration "[Channel1]" = 0.0533935 
Debug [Main]: "/home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 09 Vision.flac" fromTrackDuration = 56.1867 
Debug [Main]: "/home/be/music/yes/Youssoupha Sidibe - Sacred Sound/Youssoupha Sidibe - Sacred Sound - 08 Salaam.flac" toTrackDuration =  534.773 
Debug [Main]: m_fadeDuration "[Channel1]" = 0.0533935 
Debug [Main]: Committing transaction on "MIXXX-1" result: true 
Debug [Main]: PlaylistTableModel(0x24dc140) select() took 0 ms 3 
fish: “mixxx --logLevel debug --resour…” terminated by signal SIGSEGV (Address boundary error)

Be-ing · 2018-01-19T02:55:27Z

If anyone wants to help this along, please test it while running Mixxx in a debugger and post a backtrace if you find a crash.

nopeppermint · 2018-01-19T22:15:26Z

-there is no preview deck visible with skin latenight

gramanas · 2018-01-19T23:17:24Z

Maybe we should merge this, so people can work on each problem individually! I don't think I can correct every issue!

naught101 · 2018-02-12T02:25:55Z

Can confirm that Gramanas's steps don't cause a crash for me any more. I will test some more when I get home. 👍

naught101 · 2018-02-12T06:06:10Z

Sorting of history for parent folders is wrong. For example if I have history folders like:

2018-02-12:
#2 song D
#1 Song C

2018-02-11:
#2 song B
#1 Song A

Then the sorting for 2018/February is like this:

#2 song D
#2 song B
#1 Song C
#1 Song A

But it should be like

#2 song D
#1 Song C
#2 song B
#1 Song A

Or even better, like:

#4 song D
#3 Song C
#2 song B
#1 Song A

naught101 · 2018-02-12T06:06:26Z

Same applies to years

naught101 · 2018-02-12T06:48:01Z

Could the History icon be something like this?

Except maybe flipped horizontally, so it's going backwards?

Also the playlist icon might be clearer if it had a second shadow line.

daschuer · 2018-02-12T07:08:51Z

Could the History icon be something like this?

This looks more like a stop watch, to measure a short period of time. Something like a wall clock will work for me. This school go to master, if we find one.

naught101 · 2018-02-12T22:06:12Z

True. Something like this then?

more options: google image search "hisotry icon"

naught101 · 2018-02-12T23:26:28Z

Could the "big library" button be moved up to the top right of the library (see arrow).

Also, there is a skin glitch at smaller resolutions - see the circled bit (this shot is from a 1024px wide window). Ideally the icons section on the left would be a lot narrower, so more of the history/tracks/etc folder names can be seen.

naught101 · 2018-02-13T00:29:10Z

Another segfault. Not sure exactly what the trigger was this time, was in the background again, but here's a backtrace.

mixxx_debug.log

Be-ing · 2018-02-13T00:34:35Z

@naught101: When Mixxx crashed, before you typed bt at the GDB prompt, what was the error shown? For example before it was:

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
Library::getActiveView (this=0x0) at src/library/library.cpp:226
226         LibraryPaneManager* pPane = m_panes.value(m_focusedPaneId);

naught101 · 2018-02-13T01:11:21Z

Sorry, I missed it. I'll see if it happens again, and let you know.

naught101 · 2018-02-13T01:47:30Z

FWIW, this layout is harder to use in some ways than the master layout. In particular, it's hard to search through the main library, and then drag stuff into crates/playlists (I guess the context menu gets around that though). Is the #991 2-pane view still likely to happen after this is merged?

naught101 · 2018-02-13T02:02:07Z

Hrmm. It's also hard to see which tracks a song is in now - in the master layout, playlists and crates are highlighted when a track is selected. That isn't possible now, and the s-pane view won't help, either, I think, because it won't show a list of crates/playlists...

Would it maybe be possible to pin one of the library sections in the left browser pane, while showing a different section in the main library window? Could get confusing.. Not sure what a better solution would be..

Be-ing · 2018-02-13T02:14:23Z

Yes we will make a two feature side-by-side layout. There may be some awkward edges that won't make sense until we have a side-by-side design.

poelzi · 2018-02-13T12:02:19Z

I have a dev branch that implements a crate widget that can be shown next to the album cover. it shown only the crates as compact list the current selection is in. it works nicely so far, the only problem is multi track selections which needs some fixes. I'm planning to get this landed after this branch gets merged. kind regards poelzi

…

On 13 February 2018 03:02:08 CET, naught101 ***@***.***> wrote: Hrmm. It's also hard to see which tracks a song is in now - in the master layout, playlists and crates are highlighted when a track is selected. That isn't possible now, and the s-pane view won't help, either, I think, because it won't show a list of crates/playlists... Would it maybe be possible to pin one of the library sections in the left browser pane, while showing a different section in the main library window? Could get confusing.. Not sure what a better solution would be.. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1474 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

gramanas · 2018-02-15T18:19:32Z

@poelzi Since nested crates #1304 will replace the normal crates, some of the functionality might change, will your widget work then?

naught101 · 2018-02-16T04:37:03Z

Here's another segfault, with full log and backtrace this time. Sorry, mixxx was just running in the background, I don't know if there's anything in particular I did before that that triggered it. I had been using searching and sorting in the main track view a bit.

gdb.log

Be-ing · 2018-02-16T05:10:51Z

Oh, I think this is the same issue in master that is being solved with #1492. @uklotzde could you take a look at the backtrace and confirm that?

uklotzde · 2018-02-16T10:11:45Z

Of course, the crash in replaceRecentTrack() will be fixed with #1492 . I can only strongly recommend to merge ~~this~~that PR.

gramanas · 2018-02-16T10:24:44Z

Once it's merged in master I'll get it here as well!

gramanas · 2018-02-25T21:02:11Z

[  FAILED  ] EngineBufferE2ETest.BasicProcessingTest
[  FAILED  ] EngineBufferE2ETest.ScratchTest
[  FAILED  ] EngineBufferE2ETest.KeylockReverseTest

Those tests fail on my local machine.
They also fail in master.

Also LateNight skin is kinda broken as the recent 2.1 changes interfere with the changes jmigual introduced 2 years ago.

uklotzde · 2018-02-25T21:16:12Z

We have temporarily disabled the EngineBuffer.KeylockReverseTest test in 2.1 that is known to suffer from race conditions frequently. All tests using CachingReader may fail depending on the machine where and when they are executed.

ronso0 · 2018-02-27T00:38:26Z

Also LateNight skin is kinda broken as the recent 2.1 changes interfere with the changes jmigual introduced 2 years ago.

I can have a look at LateNight next week.

ronso0 · 2018-03-09T18:17:41Z

Sorry, the bug with LateNight (invisible Labels etc.) was due to some test changes I accidentially commited when working on Tango. I reverted them. @gramanas PR incoming

But there's an apparently hard-coded, irreversible padding in the library sidebar and library table.
This padding only appears if those components are instantiated via <LibrarySidebar> and <Library> (Deere & LateNight).
If every component is instantiated on its own like <LibrarySidebarButtons>, <LibrarySidebarExpanded>, <LibraryBreadCrumb> etc. (Shade & Tango) qss is respected.

naught101 · 2018-04-04T04:51:46Z

In the lastest version of this branch, there is an extra library column in the history view call "28", which is empty for all tracks, as far as I can see.

I've been using this branch a lot lately, and it's very stable now. 👍

gramanas · 2018-04-05T08:14:56Z

@naught101 I noticed that "28" column too, but I tottally forgot to mention it somewhere.

Right now master has got some skin patches for 2.1 and there are some merge conflicts, prohibiting me to merge master here. If anyone could do it and maybe submit a PR here it would be great.

IIRC this should have been merged by the end of March.

Keeping up with a +1,259,235 −240,583 lines PR is not the easiest thing, as I am constatly afraid I might break something. Recent PR's introduced changed to library.cpp as well that have conflicts with the changed jmigual introduced 2 years ago.

So please, if not master, can we at least merge this in another upstream branch (eg jmigual-lib-redesign)

daschuer · 2018-04-06T05:38:20Z

Sorry, we had some delays with releasing 2.1. The release will be next week than merging this into master should be our pio.
Before doing anything towards QT5 which is our second big thing for 2.2

daschuer · 2018-04-17T06:10:16Z

https://github.com/mixxxdj/mixxx/tree/jmigual-library-redesign is up to date with master now.
It looks like both branches are divergent now it looks like we did the same work twice. Can you rebase your changes onto the upsteram/jmigual-library-redesign?
Last common commit is d9415cf

gramanas changed the title ~~[WIP] Merge master to library-redesign~~ Merge master to library-redesign Jan 7, 2018

Be-ing added this to the 2.2.0 milestone Feb 25, 2018

gramanas closed this Apr 17, 2018

gramanas force-pushed the lib-redesign-conflicts branch from 296b797 to d349e3d Compare April 17, 2018 17:03

Uh oh!

Conversation

gramanas commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gramanas commented Jan 6, 2018

Uh oh!

Be-ing commented Jan 6, 2018

Uh oh!

poelzi commented Jan 7, 2018

Uh oh!

gramanas commented Jan 7, 2018

Uh oh!

Be-ing commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gramanas commented Jan 7, 2018

Uh oh!

sblaisot commented Jan 7, 2018

Uh oh!

gramanas commented Jan 7, 2018

Uh oh!

daschuer commented Jan 7, 2018

Uh oh!

daschuer commented Jan 7, 2018

Uh oh!

daschuer commented Jan 7, 2018

Uh oh!

daschuer commented Jan 7, 2018

Uh oh!

gramanas commented Jan 8, 2018

Uh oh!

gramanas commented Jan 8, 2018

Uh oh!

daschuer commented Jan 8, 2018

Uh oh!

daschuer commented Jan 8, 2018

Uh oh!

daschuer commented Jan 9, 2018

Uh oh!

Be-ing commented Jan 9, 2018

Uh oh!

gramanas commented Jan 9, 2018

Uh oh!

daschuer commented Jan 9, 2018

Uh oh!

gramanas commented Jan 9, 2018

Uh oh!

Be-ing commented Jan 11, 2018

Uh oh!

gramanas commented Jan 11, 2018

Uh oh!

Be-ing commented Jan 11, 2018

Uh oh!

Be-ing commented Jan 11, 2018

Uh oh!

Be-ing commented Jan 11, 2018

Uh oh!

Be-ing commented Jan 19, 2018

Uh oh!

nopeppermint commented Jan 19, 2018

Uh oh!

gramanas commented Jan 19, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

daschuer commented Feb 12, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

naught101 commented Feb 12, 2018

Uh oh!

naught101 commented Feb 13, 2018

Uh oh!

gramanas commented Jan 6, 2018 •

edited

Loading

Be-ing commented Jan 7, 2018 •

edited

Loading

uklotzde commented Feb 16, 2018 •

edited

Loading

gramanas commented Feb 25, 2018 •

edited

Loading

ronso0 commented Mar 9, 2018 •

edited

Loading