Skip to content

OS Theme shining though fix #1691

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

Merged
merged 4 commits into from
Jun 9, 2018
Merged

OS Theme shining though fix #1691

merged 4 commits into from
Jun 9, 2018

Conversation

daschuer
Copy link
Member

This fixes https://bugs.launchpad.net/mixxx/+bug/1773709, and should help in
#1687
and
#1677

@daschuer daschuer force-pushed the lp1773709 branch 2 times, most recently from e80beb8 to 4024830 Compare May 27, 2018 22:23
@ronso0
Copy link
Member

ronso0 commented May 28, 2018

Thanks, it works!
2 1 1-skin-fixes-tango-preview-now bg-fix

@daschuer
Copy link
Member Author

I have cleaned up the solution a bit.
Who can help with test and review?

@nopeppermint
Copy link
Contributor

still working, no OS theme shining though, but I now get more frequently this behavior:
two orange play buttons...
preview

@ronso0
Copy link
Member

ronso0 commented May 31, 2018

still working, no OS theme shining though, but I now get more frequently this behavior:
two orange play buttons...

I can see the previously activated preview button light up shortly when I hit another preview button.
But I noticed it before those changes.

@nopeppermint
Copy link
Contributor

button light up shortly

it's not shortly, it's persistent..

@nopeppermint
Copy link
Contributor

and I do not have this on #1687

@nopeppermint
Copy link
Contributor

button light up shortly

it's not shortly, it's persistent..

prober rebuild fixed everything ..

@ronso0
Copy link
Member

ronso0 commented May 31, 2018

@daschuer Before merging this, I think it's better to reset to d386676 to avoid conflicts in #1677

@nopeppermint
Copy link
Contributor

BTW:
there is more OS Theme shining through if you edit in the library:
os_theme
but in the bpm folder it's already fixed
bpm_fixed

@ronso0
Copy link
Member

ronso0 commented May 31, 2018

there is more OS Theme shining through if you edit in the library

I tried QTableView::item:edit-focus but that didn't work.

@daschuer
Copy link
Member Author

daschuer commented Jun 1, 2018

OK, rebased, to get rid of merge conflicts.
Does anyone likes to do a code review.
I like to release 2.1 soon, and it would be a pity if this will not be included.

@@ -75,6 +77,21 @@ void BPMDelegate::paint(QPainter* painter,const QStyleOptionViewItem &option,
QStyleOptionViewItemV4 opt = option;
initStyleOption(&opt, index);

// Set the palette appropriately based on whether the row is selected or
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are duplicated in TableItemDelegate::paint(). Can we move them into a protected utility function in the base class?

@uklotzde
Copy link
Contributor

uklotzde commented Jun 4, 2018

I'm not a UI expert, but the changes look fine so far. Code quality has been improved. Only a small amount of code duplication that could be avoided. I will test this now.

@uklotzde
Copy link
Contributor

uklotzde commented Jun 4, 2018

Here is a first screenshots from an inofficial Qt5 build. Later I will switch back to the Qt4 build and test again.

When selecting a BPM cell the value becomes invisible:
bpm locked selected

This worked correctly before applying these changes.

@nopeppermint
Copy link
Contributor

@uklotzde

When selecting a BPM cell the value becomes invisible:

In my opinion you need as well those UI changes from #1677

@uklotzde
Copy link
Contributor

uklotzde commented Jun 5, 2018

On Qt4 I don't even notice any visual differences in Deere (all previous changes included). My only complaint is the small amount of duplicated code.

@daschuer
Copy link
Member Author

daschuer commented Jun 5, 2018

Good catch, it was not only duplicated code It was redundant code. I have probably failed to remove it after moving the code.

@daschuer
Copy link
Member Author

daschuer commented Jun 7, 2018

@uklotzde: ready for merge?

@uklotzde
Copy link
Contributor

uklotzde commented Jun 9, 2018

Windows build failures seem to be unrelated.

I did not notice the bugs in my personal setup and configuration, but I trust the other reviewers that the original issues have been fixed. The code improvements are some welcome added value.

LGTM.

@uklotzde uklotzde merged commit c42df6e into mixxxdj:2.1 Jun 9, 2018
@Be-ing Be-ing added the ui label Jun 13, 2018
@daschuer daschuer deleted the lp1773709 branch September 7, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants