New Library Tree #11

jmigual · 2016-07-07T12:41:03Z

Hi this is the continuation of #10 since this won't be merged in the current master pull request. For the moment I've created the maintenance feature (icon included 😊 )

daschuer · 2016-07-07T13:41:11Z

Good Idea switching to a new PR ... and nice Icon :-)

daschuer · 2016-07-10T16:33:51Z

Nice progress! Here some findings:

The interaction with the search bar can be improved.
I am not sure If I should like the fact, that the query string is visible.
What are the pros and cons?
The current solution is probably OK, since the user will unlikely need to add additional search phrases if he navigates though the tree. If he decides to search other properties he is instantly aware that there is already a filter active.
However, the search bar has to interact reasonable with the tree, which makes this solution more difficult. For example, the selected three view item should move, if the user edits the search query.
Currently an active search filter is replaced when the user navigates the tree. Is this ok?

The artists should be sorted with locale aware. A Ö should be sorted next to O.

Hidden and missing tracks should not be listed in the tree

daschuer · 2016-07-10T16:49:27Z

The tree options are working. However I think it can be improved if we could distinguish sampler from single Artist albums and singles.
The "Album -> Artist" Sorting does not make much sense for single Artist albums.
How about use always Artist first and add a "Various" dummy Artist, to group samplers?
We may also consider to add an "Singles" dummy album to Group the tracks with only one Track per album. What do you think about it?
(I have a similar folder tree on my HD and it works nice for me, but I am biased by that :-*)

daschuer · 2016-07-10T16:52:22Z

There is a segfault with the HistoryFeature, when I try to play a track:

Program received signal SIGSEGV, Segmentation fault.
0x000000000097ca9f in HistoryFeature::slotPlayingTrackChanged (
this=this@entry=0x387ec10, currentPlayingTrack=...)
at src/library/historyfeature.cpp:266
266 if (m_pPlaylistTableModel->getPlaylist() == m_playlistId) {
(gdb) bt
#0 0x000000000097ca9f in HistoryFeature::slotPlayingTrackChanged (
this=this@entry=0x387ec10, currentPlayingTrack=...)
at src/library/historyfeature.cpp:266
#1 0x00000000009a82a2 in HistoryFeature::qt_static_metacall (_o=0x387ec10,
_c=, _id=, _a=0x7fffffffd730)
at lin64_build/library/moc_historyfeature.cc:68
#2 0x00007ffff4fd887a in QMetaObject::activate (

daschuer · 2016-07-10T19:06:24Z

There is an issue with changed metadata. If the Artist is edited by the user, the track becomes dirty and is not immediately stored into the database.
In the Track table we resort those tracks after the query. I am afraid this is required here as well.
Or could we just ignore it? What do you think?

jmigual · 2016-07-10T20:05:46Z

We should update the tree as well, it will be a bit hard but it's not impossible

jmigual · 2016-07-11T15:32:54Z

The interaction with the search bar can be improved.
I am not sure If I should like the fact, that the query string is visible.
What are the pros and cons?
The current solution is probably OK, since the user will unlikely need to add additional search phrases if he navigates though the tree. If he decides to search other properties he is instantly aware that there is already a filter active.

I like viewing the query string, most users probably don't know about the query strings and with this maybe they will be able to know its existence, how to use it and how powerful this can be.

However, the search bar has to interact reasonable with the tree, which makes this solution more difficult. For example, the selected three view item should move, if the user edits the search query.

This can be too hard to implement since we need to try to match the current search query with the tree view and this is a lot of work (in programming and in string evaluation). I suggest to clear the tree selection when the user enters a search query. This should help the user being aware that the search query he's doing is different from the tree search query.

Currently an active search filter is replaced when the user navigates the tree. Is this ok?

For me it's okay and it's very logical since the tree is a form of "query editor"

The artists should be sorted with locale aware. A Ö should be sorted next to O.

I'll fix this bug as soon as possible

Hidden and missing tracks should not be listed in the tree

I'll fix this too

The tree options are working. However I think it can be improved if we could distinguish sampler from single Artist albums and singles.

Okay we could also add the number of tracks in each item of the tree.

The "Album -> Artist" Sorting does not make much sense for single Artist albums.
How about use always Artist first and add a "Various" dummy Artist, to group samplers?

I like the Various idea but I prefer keeping the option to show Albums in the first level of the tree. We can remove the artist in the second level since it's not necessary but the album in the first level allows the user to view all the albums in his library with the cover art.

We may also consider to add an "Singles" dummy album to Group the tracks with only one Track per album. What do you think about it?

This is a bit dangerous since we will not be able to distinguish from singles or from albums that only have a track in the DJ library but they are not really singles.

jmigual · 2016-07-11T17:09:30Z

I can't reproduce the History Feature SIGSEGV

daschuer · 2016-07-11T22:24:10Z

The segfault is still there using the Deere Skin. m_pPlaylistTableModel is null.
It happens reliable when I am trying to play the first track after restarting Mixxx.
LateNight is not effected.

Header sorting is broken.

I like viewing the query string, most users probably don't know about the query strings and with this maybe they will be able to know its existence, how to use it and how powerful this can be.

OK, now the search bar behaves reasonable. Just deleting the selection is a good compromise.

The artists should be sorted with locale aware. A Ö should be sorted next to O.

I'll fix this bug as soon as possible

You can probably reuse the solution from here:
https://github.com/mixxxdj/mixxx/blob/7c58008d3ff35026763fdf7198905b068435e07e/src/library/basesqltablemodel.cpp#L489

Okay we could also add the number of tracks in each item of the tree.

Create Idea! This takes the pain from the issue.

The "Album -> Artist" Sorting does not make much sense for single Artist albums.
How about use always Artist first and add a "Various" dummy Artist, to group samplers?

I like the Various idea but I prefer keeping the option to show Albums in the first level of the tree. We can remove the artist in the second level since it's not necessary but the album in the first level allows the user to view all the albums in his library with the cover art.

OK, that makes sense.

We may also consider to add an "Singles" dummy album to Group the tracks with only one Track per album. What do you think about it?

This is a bit dangerous since we will not be able to distinguish from singles or from albums that only have a track in the DJ library but they are not really singles.

We may treat Albums with on Track as single. We may treat also Tracks from a Sampler that is grouped under an Artist as Single.
This would remove some clutter from Artists where the tracks are all taken from a sampler.
However we loose the "feature" to have a Single cover displayed in the tree.
I am unsure.

It looks like the MiniView Scrollbar will also help for the Treeview

jmigual · 2016-07-12T09:31:09Z

Ok, finally fixed the bug! I was looking in the wrong place to replicate it...

By the way with the history feature I think that we could avoid creating a new playlist if there's no track played. Now that I'm testing I have about 40 history playlists that are empty and from time to time I have to enter to the SQL database and delete it manually. I find this annoying...

daschuer · 2016-07-12T10:29:02Z

I can remember to discuss this already some times ago, it is a good idea.
We have also discussed to put the recent playlist on top.

A full featured solution will be something like this:
https://bugs.launchpad.net/mixxx/+bug/1127120

jmigual · 2016-07-12T17:34:20Z

We may also consider to add an "Singles" dummy album to Group the tracks with only one Track per album. What do you think about it?

This is a bit dangerous since we will not be able to distinguish from singles or from albums that only have a track in the DJ library but they are not really singles.

We may treat Albums with on Track as single. We may treat also Tracks from a Sampler that is grouped under an Artist as Single.
This would remove some clutter from Artists where the tracks are all taken from a sampler.
However we loose the "feature" to have a Single cover displayed in the tree.
I am unsure.

I just noticed we can't add the "singles" item because the search bar currently does not support this. So the current search SQL query and the search bar will not be consistent and I think that this can confuse the user. Another option is to add support for OR queries in the search bar but this can take many time and I prefer creating the MiniView scrollbar first.

daschuer · 2016-07-12T19:53:00Z

The missing tracks are still in the library tree

daschuer · 2016-07-12T19:59:26Z

Now "Ö" is treated as "O" when sorting. If Ö is the first letter, we have three O like first letter sections.
O Ö and again O.

An extra Ö section is OK, but we should merge at least both O sections. Or we can group all tracks at O.

jmigual · 2016-07-13T07:37:34Z

I prefer grouping all tracks at O. It shows less headers and it's not necessary a different header for every special character...

daschuer · 2016-07-13T12:22:06Z

src/library/librarytreemodel.cpp

+                if (c.isLetter()) {
+                    // We only can remove the accents if its a latin character
+                    if (c.toLatin1() != 0) {
+                        QString s1 = value.normalized(QString::NormalizationForm_KD);


This is a good candidate to put it into a helper function and add a unit-test for it. :-)

However, your solution does not work for non latin 1 charters like "Ő"

There seams also to be an issue for Finish sorting.

I have just found this post (sorry):
http://stackoverflow.com/questions/3773914/why-isnt-qstringlocaleawarecompare-working-correctly

This means that we cannot sort å at a on finish systems because the Finish native speakers will search it below Z.

How about replacing the
if (lastHeader != c) {
somhow with
QString::localeAwareCompare( lastHeader, c ) != 0
to solve the finish case.

Then we have "only" the issue to combine neighboring characters under the Ascii Type and not under a "Ä" if it comes first.
Maybe we can use your formula above and compare the result with localeAwareCompare()
The latin1 check can probably be removed, since localAwareCompare will sort those issues out.

Mmmm the latin1 check was added specially for chinese/japanese characters. If I remove all non [A-Z] characters in chinese a null string will appear... I am still finding something to check it... Maybe adding another regular expression will solve it.

If your function returns null, I think you can safely use the original Character.
Because of the Finish sorting issue, we have to check anyway if the original and the compatible character are sorted the same. This check will fail for Chinese.

jmigual · 2016-07-14T20:19:30Z

Hi, I've added the mini map in the scrollbar what do you think about it?

daschuer · 2016-07-14T22:10:09Z

Wow the Latenight version looks very slick. The Deere an Schade versions are not yet styled, right?

How does the styling work? Are the characters painted on below a semi transparent scroll handler?

It looks like the space for the up and down buttons at the top and bottom, visible in Shade is not taken into account. It would be OK for me to force remove them, if that is the easiest solution.

I had expected, that if you click on a character, that the scroll handle moves to this position.
This is the default in Gtk applications. In Qt, it does a page up and page down instead.
Is it possible to change it for the new scroll bar?

The positioning function should be able to skip items if they will consume the ideal place of the following item. The issue can be seen, if you open a playlist and sort by # . If you now shrink the library hight, the 9 for example is now displayed under the slider at 16.

If sorted by # we should consider a strategy to avoid "1234567891 2"
How about always use the left most number. In the case above it becomes "0 1 2"

After all this testing, I am starting to miss this bar in other apps like Thunderbird for instance :-))

jmigual · 2016-07-15T06:51:26Z

The currently styled version is the Deere skin. To style it it's like any other widget, to change the font color only has to use the color rule and to change the font size font-size: rule.

It looks like the space for the up and down buttons at the top and bottom, visible in Shade is not taken into account. It would be OK for me to force remove them, if that is the easiest solution.

Ok I'll remove the up and down buttons

I had expected, that if you click on a character, that the scroll handle moves to this position.
This is the default in Gtk applications. In Qt, it does a page up and page down instead.
Is it possible to change it for the new scroll bar?

Yes I think that it's possible to change it.

The positioning function should be able to skip items if they will consume the ideal place of the following item. The issue can be seen, if you open a playlist and sort by # . If you now shrink the library hight, the 9 for example is now displayed under the slider at 16.

Ok the positioning function is a bit complicated. I have to deal with the space occupied by each letter and change it if it is less than the size of a letter (the results were terrible when not doing that). You can find the function in WMiniViewScrollBar::computeLetterSize.

If sorted by # we should consider a strategy to avoid "1234567891 2"
How about always use the left most number. In the case above it becomes "0 1 2"

Ok, good idea, we should also decide where the mini view should show the numbers because if you sort by cover art nearly random numbers appear and they have no sense (it would be really cool to show the cover art too in the scrollbar).

daschuer · 2016-07-15T08:11:30Z

It looks like you paint the characters on top of all.
Is it possible, to paint it under the scroll handler, and make the scroll handler semi transparent?

daschuer · 2016-07-15T08:21:32Z

src/widget/wminiviewscrollbar.h

+
+    bool m_showLetters;
+    QVector<QPair<QChar, int> > m_letters;
+    QVector<QPair<QChar, int> > m_computedSize;


QPair is evil and should be IMHO removed from Qt ;-)
Please replace it with a private struct or class.

jmigual · 2016-07-15T08:40:39Z

It should be possible, in kate they do it like you said

daschuer · 2016-07-15T08:44:17Z

src/widget/wminiviewscrollbar.cpp

+    m_computedSize = m_letters;
+    m_lettersMutex.unlock();
+
+    if (!enoughSpace) {


This algorithm need to be tweaked.
It so not important to have many letter visible, it is important that it sticks on the right position.

How about this:

Always paint the topmost letter at the top, this will consume letterSize, which is probably more then the available letter Space. Skipp all following letters, which will be painted overlapping on the first letter.

Steps:

Loop through m_computedSize

Add visible letters letterSize to nextAvailableScrollPosition;

Add all letterSpaces (second) to optimalScrollPosition;

If optimalScrollPosition < nextAvailableScrollPosition, remove the letter.

This should ensure, that the top of the scrollbar matches always the top of the letter, which is on top of the visible area.

Ok, this should work but what about letters that do not fit the size by 1 or 2 pixels? We should take in account such cases (1 pixel misaligned it's not a big problem). I think that this function will be changing many time until it's perfect 😅

daschuer · 2016-07-15T08:49:27Z

In case of sorting by year, we have only 1 and 2. This should be special cased as well to return the decade.

jmigual · 2016-07-15T12:51:22Z

Hi Daniel, I've tried to show the slider transparent and the letters below but it's complicated and it implies rewriting all the paint event.

daschuer · 2016-07-15T14:10:02Z

Did you consider to just copy the paint event from the original qt source
and modify it?
I have not yet looked to the source, but it could be probably complicated
because of the difference target OS styles
If there is a point in the parent class before painting the slider where we
can install a hook for your code, we have won.

Am 15.07.2016 2:51 nachm. schrieb "Joan" [email protected]:

Hi Daniel, I've tried to show the slider transparent and the letters below
but it's complicated and it implies rewriting all the paint event.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABsfIph8QtNn3uY7NHJaeUlp60tKjSrYks5qV4JKgaJpZM4JHCXW
.

jmigual · 2016-07-15T15:11:28Z

Oh, I haven't looked at the source, good idea.

jmigual · 2016-07-15T16:23:39Z

Ok, it's done, now with a bit of CSS I should be able to make it transparent

jmigual · 2016-07-15T19:02:36Z

Now when you click a letter it scrolls directly to the letter, there's a hover feature for visual feedback.

Conflicts: src/library/features/playlist/playlisttablemodel.cpp src/library/features/playlist/playlisttablemodel.h src/library/basesqltablemodel.h

Library focus

…plylist panes

…n deleted index.

Conflict src/library/features/playlist/playlistfeature.cpp

Library focus

fix highlighted xfader orientation buttons

Merge master + fix include directives

…h sync When loading a track that is not yet present in the library (and thus doesn't have any BPM because it hasn't been analyzed yet) while another deck is playing and both decks have sync enabled, a debug assertion is triggered: DEBUG ASSERT: "isValid()" in function double mixxx::Bpm::value() const at src/track/bpm.h:53 Aborted (core dumped) The backtrace looks as follows: #0 0x00007f175c87234c in __pthread_kill_implementation () at /usr/lib/libc.so.6 #1 0x00007f175c8254b8 in raise () at /usr/lib/libc.so.6 #2 0x00007f175c80f534 in abort () at /usr/lib/libc.so.6 #3 0x00007f175cf05ee4 in qt_assert(char const*, char const*, int) () at /usr/lib/libQt5Core.so.5 #4 0x000055deb2e67e1c in mixxx::(anonymous namespace)::handleMessage(QtMsgType, QMessageLogContext const&, QString const&) (type=<optimized out>, context=<optimized out>, input=<optimized out>) at src/util/logging.cpp:355 #5 0x00007f175cf47128 in () at /usr/lib/libQt5Core.so.5 #6 0x00007f175cf3fd8a in () at /usr/lib/libQt5Core.so.5 #7 0x00007f175cf06526 in QMessageLogger::critical(char const*, ...) const () at /usr/lib/libQt5Core.so.5 #8 0x000055deb2e5c720 in mixxx_debug_assert(char const*, char const*, int, char const*) (assertion=assertion@entry=0x55deb39bd0db "isValid()", file=file@entry=0x55deb39bbf30 "src/track/bpm.h", line=line@entry=53, function=function@entry=0x55deb39bbf08 "double mixxx::Bpm::value() const") at gsrc/util/assert.h:9 #9 0x000055deb2ee7e7e in mixxx_debug_assert_return_true(char const*, char const*, int, char const*) (function=0x55deb39bbf08 "double mixxx::Bpm::value() const", line=53, file=0x55deb39bbf30 "src/track/bpm.h", assertion=0x55deb39bd0db "isValid()") at gsrc/util/assert.h:18 #10 mixxx::Bpm::value() const (this=<synthetic pointer>) at src/track/bpm.h:53 #11 mixxx::operator*(mixxx::Bpm, double) (multiple=1, bpm=...) at src/track/bpm.h:160 #12 SyncControl::setLocalBpm(mixxx::Bpm) (this=<optimized out>, localBpm=...) at src/engine/sync/synccontrol.cpp:567 #13 0x000055deb34c7ba3 in EngineBuffer::postProcess(int) (this=0x55deb56eb060, iBufferSize=2048) at src/engine/enginebuffer.cpp:1318 #14 0x000055deb3139023 in EngineMaster::processChannels(int) (this=0x55deb5449440, iBufferSize=<optimized out>) at src/engine/enginemaster.cpp:383 #15 0x000055deb31394f7 in EngineMaster::process(int) (this=0x55deb5449440, iBufferSize=iBufferSize@entry=2048) at src/engine/enginemaster.cpp:410 #16 0x000055deb2f91d0b in SoundManager::onDeviceOutputCallback(long) (this=<optimized out>, iFramesPerBuffer=iFramesPerBuffer@entry=1024) at src/soundio/soundmanager.cpp:596 #17 0x000055deb32dd794 in SoundDevicePortAudio::callbackProcessClkRef(long, float*, float const*, PaStreamCallbackTimeInfo const*, unsigned long) (this=0x55deb553e6b0, framesPerBuffer=1024, out=<optimized out>, in=<optimized out>, timeInfo=<optimized out>, statusFlags=<optimized out>) at src/soundio/sounddeviceportaudio.cpp:965 This happens because `newLocalBpm` is invalid when `localBpm` is invalid. Trying to do sync decks while no tempo information is available does not make sense, so we only synchronize decks if the local BPM is available.

daschuer reviewed Jul 13, 2016
View reviewed changes

daschuer reviewed Jul 15, 2016
View reviewed changes

daschuer and others added 25 commits November 30, 2016 23:28

Fix selection style in Treeview of LateNight

20e3143

Skip header when scrolling by up/down

3fe2902

Merge branch 'daniel/master' into feature/libraryTree

96f0d6d

Merge master

23756c5

Conflicts: src/library/features/playlist/playlisttablemodel.cpp src/library/features/playlist/playlisttablemodel.h src/library/basesqltablemodel.h

Remove foreach

be6b81d

Fix some compilation issues

ac00985

Added new image for locked preselect button

f02a996

Fix build

645cd28

Merge pull request #3 from daschuer/libraryFocus

8d539cd

Library focus

LibrarySidebarExpanden now moved the preselected button as well

755afc4

joined featurePane and SavedPane, this allows better control for the …

0cb8452

…plylist panes

Invalidate stored Index when skin is changing, fixes a crasher.

d05ec06

Fix invalid restoreSearch during skin setup

79d4876

Avoid to create the library base track chache twice.

4e97415

Remove unnecessary Assert

993e87b

Improved Deere syling a bit

696d5d3

Add focus borders to Shade skin

6efa706

Merge remote-tracking branch 'mixxxdj/master' into feature/libraryTree

8f6035f

Fix windows compilation

e98b719

Silence some debug messages

84f1929

Added beginResetModel endResetModel regions to avoid crashes due to a…

d16b7c4

…n deleted index.

Fix windows build

8e4485b

Merged branch master into feature/libraryTree

6e12936

Conflict src/library/features/playlist/playlistfeature.cpp

Fix windows compilation with Qt4 and initializer lists

73094d0

Merge pull request #4 from daschuer/libraryFocus

c028f29

Library focus

jmigual closed this Jan 15, 2017

jmigual deleted the feature/libraryTree branch January 24, 2017 21:40

daschuer pushed a commit that referenced this pull request Dec 11, 2017

Merge pull request #11 from ronso0/Deere-2.1-update2

6cd5373

fix highlighted xfader orientation buttons

daschuer pushed a commit that referenced this pull request Oct 16, 2019

Merge pull request #11 from uklotzde/new-signals-slots-syntax-library

8599945

Merge master + fix include directives

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Library Tree #11

New Library Tree #11

jmigual commented Jul 7, 2016

daschuer commented Jul 7, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

jmigual commented Jul 10, 2016

jmigual commented Jul 11, 2016

jmigual commented Jul 11, 2016

daschuer commented Jul 11, 2016

jmigual commented Jul 12, 2016

daschuer commented Jul 12, 2016

jmigual commented Jul 12, 2016

daschuer commented Jul 12, 2016

daschuer commented Jul 12, 2016

jmigual commented Jul 13, 2016

daschuer Jul 13, 2016

jmigual Jul 13, 2016

daschuer Jul 13, 2016

jmigual commented Jul 14, 2016

daschuer commented Jul 14, 2016

jmigual commented Jul 15, 2016

daschuer commented Jul 15, 2016

daschuer Jul 15, 2016

jmigual commented Jul 15, 2016

daschuer Jul 15, 2016

jmigual Jul 15, 2016

daschuer commented Jul 15, 2016

jmigual commented Jul 15, 2016

daschuer commented Jul 15, 2016

jmigual commented Jul 15, 2016

jmigual commented Jul 15, 2016

jmigual commented Jul 15, 2016

New Library Tree #11

New Library Tree #11

Conversation

jmigual commented Jul 7, 2016

daschuer commented Jul 7, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

daschuer commented Jul 10, 2016

jmigual commented Jul 10, 2016

jmigual commented Jul 11, 2016

jmigual commented Jul 11, 2016

daschuer commented Jul 11, 2016

jmigual commented Jul 12, 2016

daschuer commented Jul 12, 2016

jmigual commented Jul 12, 2016

daschuer commented Jul 12, 2016

daschuer commented Jul 12, 2016

jmigual commented Jul 13, 2016

daschuer Jul 13, 2016

Choose a reason for hiding this comment

jmigual Jul 13, 2016

Choose a reason for hiding this comment

daschuer Jul 13, 2016

Choose a reason for hiding this comment

jmigual commented Jul 14, 2016

daschuer commented Jul 14, 2016

jmigual commented Jul 15, 2016

daschuer commented Jul 15, 2016

daschuer Jul 15, 2016

Choose a reason for hiding this comment

jmigual commented Jul 15, 2016

daschuer Jul 15, 2016

Choose a reason for hiding this comment

jmigual Jul 15, 2016

Choose a reason for hiding this comment

daschuer commented Jul 15, 2016

jmigual commented Jul 15, 2016

daschuer commented Jul 15, 2016

jmigual commented Jul 15, 2016

jmigual commented Jul 15, 2016

jmigual commented Jul 15, 2016