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

Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate #2538

Merged
merged 13 commits into from
Apr 6, 2020
Merged

Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate #2538

merged 13 commits into from
Apr 6, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Mar 8, 2020

Required for: #2545, #2524, #2282

I recommend to get this integrated into 2.3! Otherwise I expect a substantial amount of porting efforts in case of bug fixes in 2.3 that affect the table model and views.

  • Extracted BaseTrackTableModel from BaseSqlTableModel
  • Extracted BaseCoverArtDelegate from CoverArtDelegate
  • Emit a batch of multiple rowsChanged() from BaseCoverArtDelegate as a single signal
  • Removed redundant code from BansheePlaylistModel

This PR lays the ground to reuse our table views for external libraries that are not populated from search queries executed on the internal database. As such it enables to provide different views on the internal track collection. Track objects are then still loaded from the internal database as before.

It includes an optimization of the communication between CoverArtDelegate and the table view. Rows that have been marked as cache miss and need to be refreshed by the view are now sent by rowsChanged(QList<int> rows) all at once instead of one by one. This allows to refresh consecutive rows in the table view instead of only single cells.

@uklotzde uklotzde added this to the 2.3.0 milestone Mar 8, 2020
@uklotzde uklotzde changed the title [WiP] Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate Mar 11, 2020
@uklotzde
Copy link
Contributor Author

I guess it is better to integrate these changes before adding new features like the configurable background opacity for skins.

@Holzhaus
Copy link
Member

With this PR the row background of tracks without a color partially shows up as black (although the color column doesn't):
2020-03-13-232559_584x160_scrot

@Holzhaus
Copy link
Member

Also, Mixxx segfaults when I try to load an external Serato Library. Here's a backtrace from gdb:

─── Output/messages ────────────────────────────────────────────────────────────────────────────

Thread 1 "mixxx" received signal SIGSEGV, Segmentation fault.
0x0000555555afd6c1 in BaseSqlTableModel::rawValue(QModelIndex const&) const ()
─── Assembly ───────────────────────────────────────────────────────────────────────────────────
0x0000555555afd6b6 ? mov    0x8(%rbp),%r10
0x0000555555afd6ba ? shl    $0x4,%r13
0x0000555555afd6be ? mov    %r12,%rdi
0x0000555555afd6c1 ? mov    0x10(%r10),%rsi
0x0000555555afd6c5 ? add    %r13,%rsi
0x0000555555afd6c8 ? add    %r10,%rsi
0x0000555555afd6cb ? callq  0x5555556326e0 <_ZN8QVariantC1ERKS_@plt>
─── Expressions ────────────────────────────────────────────────────────────────────────────────
─── History ────────────────────────────────────────────────────────────────────────────────────
─── Memory ─────────────────────────────────────────────────────────────────────────────────────
─── Registers ──────────────────────────────────────────────────────────────────────────────────
   rax 0x0000000000000002          rbx 0x000055555916faa0          rcx 0x000055555f11f400
   rdx 0x000055555e460e30          rsi 0x000000000000001f          rdi 0x00007fffffffa0f0
   rbp 0x000055555f11f408          rsp 0x00007fffffffa030           r8 0x00007fffffffa070
    r9 0x000055555916faa0          r10 0x0000000000000018          r11 0x00005555563210a0
   r12 0x00007fffffffa0f0          r13 0xfffffffffffffff0          r14 0x0000000000000006
   r15 0x00007fffffffa188          rip 0x0000555555afd6c1       eflags [ CF PF AF SF IF RF ]
    cs 0x00000033                   ss 0x0000002b                   ds 0x00000000
    es 0x00000000                   fs 0x00000000                   gs 0x00000000
─── Source ─────────────────────────────────────────────────────────────────────────────────────
─── Stack ──────────────────────────────────────────────────────────────────────────────────────
[0] from 0x0000555555afd6c1 in BaseSqlTableModel::rawValue(QModelIndex const&) const
(no arguments)
[1] from 0x0000555555b1612d in BaseTrackTableModel::data(QModelIndex const&, int) const
(no arguments)
[+]
─── Threads ────────────────────────────────────────────────────────────────────────────────────
[49] id 148429 name Thread (pooled) from 0x00007ffff4401f7a in pthread_cond_timedwait@@GLIBC_2.3.2
[48] id 148428 name SoundDeviceNetw from 0x00007ffff4405d65 in nanosleep+69
[47] id 148427 name gdbus from 0x00007ffff431f9ef in poll+79
[46] id 148426 name gmain from 0x00007ffff431f9ef in poll+79
[45] id 148425 name Controller from 0x00007ffff431f9ef in poll+79
[44] id 148424 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[43] id 148423 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[42] id 148422 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[41] id 148421 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[40] id 148420 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[39] id 148419 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[38] id 148418 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[37] id 148417 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[36] id 148416 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[35] id 148415 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[34] id 148414 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[33] id 148413 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[32] id 148412 name VSyncThread from 0x00007ffff4324e9d in syscall+29
[31] id 148411 name Controller from 0x00007ffff431f9ef in poll+79
[30] id 148410 name AnalyzerThread from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[29] id 148409 name AnalyzerThread from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[28] id 148408 name BrowseThread from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[27] id 148407 name LibraryScanner  from 0x00007ffff431f9ef in poll+79
[26] id 148406 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[25] id 148405 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[24] id 148404 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[23] id 148403 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[22] id 148402 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[21] id 148401 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[20] id 148400 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[19] id 148399 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[18] id 148398 name CachingReaderWo from 0x00007ffff4324e9d in syscall+29
[17] id 148397 name VinylControlPro from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[11] id 148390 name EngineSideChain from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[10] id 148389 name EngineWorkerSch from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[9] id 148388 name mixxx:disk$3 from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[8] id 148387 name mixxx:disk$2 from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[7] id 148386 name mixxx:disk$1 from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[6] id 148385 name mixxx:disk$0 from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[5] id 148384 name mixxx:rcs0 from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[4] id 148383 name mixxx from 0x00007ffff4401c45 in pthread_cond_wait@@GLIBC_2.3.2
[3] id 148382 name QDBusConnection from 0x00007ffff431f9ef in poll+79
[2] id 148381 name QXcbEventQueue from 0x00007ffff431f9ef in poll+79
[1] id 148376 name mixxx from 0x0000555555afd6c1 in BaseSqlTableModel::rawValue(QModelIndex const&) const
────────────────────────────────────────────────────────────────────────────────────────────────

@uklotzde uklotzde changed the title Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate [WiP] Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate Mar 14, 2020
- Reduce the hard-coded dependencies to other classes to be reusable
  for integrating external track collections
- Send a single signal for marking rows as dirty or changed
@uklotzde
Copy link
Contributor Author

The crash should be fixed, just a missing index validation.

@uklotzde uklotzde changed the title [WiP] Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate Refactoring: Extract BaseTrackTableModel + BaseCoverArtDelegate Mar 14, 2020
@uklotzde
Copy link
Contributor Author

With this PR the row background of tracks without a color partially shows up as black (although the color column doesn't):
2020-03-13-232559_584x160_scrot

Fixed 🙈

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I skimmed over the code and didn't spot any obvious issues. Both bugs (the segfault and the row background problem) are fixed. However, before we merge someone else should have a look at this, too.

@Holzhaus Holzhaus requested a review from daschuer March 17, 2020 17:18
@Holzhaus
Copy link
Member

Holzhaus commented Apr 2, 2020

@mixxxdj/developers Can someone take a look? I'd merge this but since this is pretty big I want to make sure I didn't overlook something.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I retested today and it still works fine. Code also LGTM. Thank you. Since nobody pointed out any issues with this in more than three weeks, I'll go ahead and merge now.

@Holzhaus Holzhaus merged commit 6882798 into mixxxdj:master Apr 6, 2020
@uklotzde uklotzde deleted the basetracktablemodel branch April 6, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants