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

Add note to coin control dialog QT5 workaround #7256

Merged
merged 1 commit into from Jan 4, 2016

Conversation

Projects
None yet
6 participants
@fanquake
Member

fanquake commented Dec 26, 2015

I can no longer reproduce the original issue (#3846) with QT 5.5

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 26, 2015

Member

Debian is at 5.3.2 - can someone test if that needs this?

Member

luke-jr commented Dec 26, 2015

Debian is at 5.3.2 - can someone test if that needs this?

@jonasschnelli jonasschnelli added the GUI label Dec 27, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 2, 2016

Member

I guess the upstream bug is that one: https://bugreports.qt.io/browse/QTBUG-43473.
Maybe it's to early to remove it... will test on Debian and OSX with Qt<5.5...

Member

jonasschnelli commented Jan 2, 2016

I guess the upstream bug is that one: https://bugreports.qt.io/browse/QTBUG-43473.
Maybe it's to early to remove it... will test on Debian and OSX with Qt<5.5...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 2, 2016

Member

Tested on stable debian (8.x, just downloaded and did a VM net install). I guess the bug was fixed in Qt5.4. and debian is still on 5.3.x.
I guess we should keep the workaround another year.

Member

jonasschnelli commented Jan 2, 2016

Tested on stable debian (8.x, just downloaded and did a VM net install). I guess the bug was fixed in Qt5.4. and debian is still on 5.3.x.
I guess we should keep the workaround another year.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 2, 2016

Member

Without the workaround, selecting a parent will result in partial check of the parent and all it's childs (which is a bug).

bildschirmfoto 2016-01-02 um 10 11 06

Member

jonasschnelli commented Jan 2, 2016

Without the workaround, selecting a parent will result in partial check of the parent and all it's childs (which is a bug).

bildschirmfoto 2016-01-02 um 10 11 06

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Jan 2, 2016

Member

Yes that is the work around the bug is fixing, if it's still present the workaround can remain. Should we add a note that it's fixed in 5.4, and as such can be removed once all platforms have upgraded to it?

Member

fanquake commented Jan 2, 2016

Yes that is the work around the bug is fixing, if it's still present the workaround can remain. Should we add a note that it's fixed in 5.4, and as such can be removed once all platforms have upgraded to it?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 2, 2016

Member

Yes. And I think we should add a link to the upstream bug in the comment.

Member

jonasschnelli commented Jan 2, 2016

Yes. And I think we should add a link to the upstream bug in the comment.

@fanquake fanquake changed the title from Remove QT5 workaround from coin control dialog to Add note to coin control dialog QT5 workaround Jan 2, 2016

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Jan 2, 2016

Member

Changes pushed.

Member

fanquake commented Jan 2, 2016

Changes pushed.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Jan 2, 2016

ACK 33877ed

@@ -411,7 +411,7 @@ void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column)
// todo: this is a temporary qt5 fix: when clicking a parent node in tree mode, the parent node
// including all children are partially selected. But the parent node should be fully selected
// as well as the children. Children should never be partially selected in the first place.
// Please remove this ugly fix, once the bug is solved upstream.
// Should be fixed in Qt5.4 and above. https://bugreports.qt.io/browse/QTBUG-43473
#if QT_VERSION >= 0x050000

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 2, 2016

Member

What about

#if QT_VERSION >= 0x050000 && QT_VERSION <= 0x050500
@MarcoFalke

MarcoFalke Jan 2, 2016

Member

What about

#if QT_VERSION >= 0x050000 && QT_VERSION <= 0x050500

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 2, 2016

Member

Yes. We have discussed that on IRC and IMO it would look after we are overdoing the workaround.
Let's just keep it and remove the WA after one or two years.

@jonasschnelli

jonasschnelli Jan 2, 2016

Member

Yes. We have discussed that on IRC and IMO it would look after we are overdoing the workaround.
Let's just keep it and remove the WA after one or two years.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 2, 2016

Member

My reasoning was that gitian uses qt5.5, so we don't need it in the official releases. People that are using qt-5.3 or qt-5.4, still have it enabled.

@MarcoFalke

MarcoFalke Jan 2, 2016

Member

My reasoning was that gitian uses qt5.5, so we don't need it in the official releases. People that are using qt-5.3 or qt-5.4, still have it enabled.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jan 3, 2016

Contributor

I'd like to see the comment about removing updated and stay there (or moved after the link to Qt bug) - something like

Please remove this ugly fix, once fixed Qt is used in all relevant platforms.
Contributor

paveljanik commented Jan 3, 2016

I'd like to see the comment about removing updated and stay there (or moved after the link to Qt bug) - something like

Please remove this ugly fix, once fixed Qt is used in all relevant platforms.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 4, 2016

Member

utACK

Member

laanwj commented Jan 4, 2016

utACK

@laanwj laanwj merged commit 33877ed into bitcoin:master Jan 4, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 4, 2016

Merge pull request #7256
33877ed Add note to CoinControl Dialog workaround (fanquake)

@fanquake fanquake deleted the fanquake:remove-qt5-fix-ccontrol branch May 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment