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

Fix dock pane layout #545

Merged
merged 3 commits into from
Jun 30, 2020
Merged

Fix dock pane layout #545

merged 3 commits into from
Jun 30, 2020

Conversation

pedrorivotti
Copy link
Contributor

This PR is an alternative to #535 that also closes #442

@rahulporuri Could you please check if this approach works for your use case?

@rahulporuri
Copy link
Contributor

This PR definitely fixes the issue unlike #535 BUT understandably, this breaks behavior on pyqt 4 🤣

Comment on lines 157 to 159
# Remove the fixed sizes once Qt activates the layout.
QtCore.QTimer.singleShot(0, self._reset_fixed_sizes)

Copy link
Contributor

Choose a reason for hiding this comment

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

can you undo the removals and add if/else clauses to not change the behavior on qt4? like in #535 . (i haven't checked the test suite but i'd wager reverting the changes would fix the test suite)

Not sure how to write a regression test for this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

i restored the removed code and moved into a qt4 conditional branch. but i'm still not sure how to add a regression test for this.

@pedrorivotti pedrorivotti marked this pull request as draft June 17, 2020 13:03
@rahulporuri
Copy link
Contributor

@pedrorivotti will you be able to work on this PR this week? If not, I can take over and update the PR (unless someone on the ETS team can take over @corranwebster ). It'd be nice to get this merged and shipped in a bug fix soon.

@pedrorivotti
Copy link
Contributor Author

@rahulporuri Please feel free to take over, I don't think I would get around to it before next week.
Thanks!

	modified:   pyface/ui/qt4/tasks/main_window_layout.py
@rahulporuri rahulporuri marked this pull request as ready for review June 23, 2020 18:13
@rahulporuri
Copy link
Contributor

@kitchoi @corranwebster any guidance on adding a regression test for this? I have tested this locally on our application with qt4 and qt5 - and dock pane restoration works as expected on both toolkits.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2020

Codecov Report

Merging #545 into master will increase coverage by 0.21%.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
+ Coverage   36.99%   37.20%   +0.21%     
==========================================
  Files         470      470              
  Lines       26027    26096      +69     
  Branches     3961     3971      +10     
==========================================
+ Hits         9629     9710      +81     
+ Misses      15977    15970       -7     
+ Partials      421      416       -5     
Impacted Files Coverage Δ
pyface/ui/qt4/tasks/main_window_layout.py 18.07% <23.07%> (+0.05%) ⬆️
pyface/ui/qt4/util/image_helpers.py 97.14% <0.00%> (+0.08%) ⬆️
pyface/ui/qt4/dialog.py 94.66% <0.00%> (+0.72%) ⬆️
pyface/ui/qt4/about_dialog.py 84.44% <0.00%> (+2.86%) ⬆️
pyface/ui/qt4/action/tool_bar_manager.py 84.28% <0.00%> (+3.92%) ⬆️
pyface/ui/qt4/progress_dialog.py 90.71% <0.00%> (+5.10%) ⬆️
pyface/ui/qt4/action/action_item.py 46.46% <0.00%> (+5.15%) ⬆️
pyface/ui/qt4/action/menu_manager.py 70.29% <0.00%> (+7.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ba6c2...a1fe97f. Read the comment docs.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM. @corranwebster Do you want to have a look too before this goes in?

@kitchoi
Copy link
Contributor

kitchoi commented Jun 30, 2020

Confirmed with @corranwebster offline that this can be merged.

@kitchoi kitchoi merged commit fa90fa9 into master Jun 30, 2020
@kitchoi kitchoi deleted the fix/dock-layout-sizehint branch June 30, 2020 10:45
@rahulporuri
Copy link
Contributor

rahulporuri commented Jun 30, 2020

@kitchoi are there plans of a bugfix release?

@kitchoi
Copy link
Contributor

kitchoi commented Jun 30, 2020

I just opened a bug release check list issue for TraitsUI, and will open one for Pyface. It is not really a complete plan as in there isn't a target release date yet. Open for discussion!

@kitchoi kitchoi added this to the Release 7.0.1 milestone Jun 30, 2020
kitchoi added a commit that referenced this pull request Jul 8, 2020
* Provide sizeHint to QWidget

* REF : Restore qt4 conditional branch

	modified:   pyface/ui/qt4/tasks/main_window_layout.py

* TST: Test Qt dock widget has the correct size (#552)

Co-authored-by: Sai Rahul Poruri <rporuri@enthought.com>
Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
kitchoi added a commit that referenced this pull request Jul 8, 2020
* Fix errors from incorrect QImage memory management. (#546)

This should stop the sporadic test failures caused by referencing memory
which has been freed and used by something else.  The fix is to attach the
numpy array with the allocated memory to the QImage, so that the lifetime
of the array is at least as long as that of the QImage.

* Fix dock pane layout (#545)

* Provide sizeHint to QWidget

* REF : Restore qt4 conditional branch

	modified:   pyface/ui/qt4/tasks/main_window_layout.py

* TST: Test Qt dock widget has the correct size (#552)

Co-authored-by: Sai Rahul Poruri <rporuri@enthought.com>
Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* Update Changelog for 7.0.1

* Remove master only setting on Travis and Appveyor which prevent bug fix release PR from building

* Update .travis.yml for Qt (#529)

* Update .travis.yml to include libglu1-mesa-dev for Qt

* Restore libxkbcommon-x11 and move other dependencies up

* Fix package name for libsdl

* Add a debug flag

* Add libxcb-iccm4

* Continue the same exercise for pyside2, add libxcb-image

* Giving up - let's use the same packages

* Add libxml2 for osx image

* Use brew manually
See https://travis-ci.community/t/macos-build-fails-because-of-homebrew-bundle-unknown-command/7296/28

* Try a newer osx image

* Add a comment

* Is libdbus still needed?

* Remove debugging flag - it is noisy and is embedded in between tests

* Update install command for installing wx to match master (this is equivalent to a merge of a couple of PRs

Co-authored-by: Corran Webster <cwebster@enthought.com>
Co-authored-by: Pedro Rivotti <44579232+pedrorivotti@users.noreply.github.com>
Co-authored-by: Sai Rahul Poruri <rporuri@enthought.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dock pane size control fails with Qt5
4 participants