yarikoptic commented 2 years ago

It felt inconvenient to need to rescale header (columns) in the tree view each time and/or make window bigger. I guess there could be more geometries and states to store/restore but this is the main ones I felt need for.

I guess such functionality could become configurable, but as IMHO should be enabled by default for better UX decided to suggest hardcoded first.

mih commented 2 years ago

Great convenience upgrade! Thx!

It seems that the store is coming too late on deinit. not sure how exactly that happens. It may be a side-effect of the test setup.

yarikoptic commented 2 years ago

I will look into it later. Works for you ok locally?

mih commented 2 years ago

I will look into it later. Works for you ok locally?

Yes, perfect. Keep window geometry and FSBrowser column widths.

yarikoptic commented 2 years ago

I have pushed f1d1b5151b1038db4201cdd7ddec6b4f6dcb55c3 but not sure if that is the best way or you (or I) would like it - dilutes somewhat main_window logic etc. Please see commit for more description. I did try to just "monkey patch" .closeEvent upon that load_ui but for some reason it didn't quite work for some reason.

edit: ideally, I feel, that more should be concentrated in such a class/closeEvent so there would be no need for manual calling of deinit

codecov-commenter commented 2 years ago

Codecov Report

Base: 66.22% // Head: 66.58% // Increases project coverage by +0.35% :tada:

Coverage data is based on head (f1d1b51) compared to base (01cf641). Patch coverage: 94.59% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #282 +/- ## ========================================== + Coverage 66.22% 66.58% +0.35% ========================================== Files 39 42 +3 Lines 2209 2948 +739 ========================================== + Hits 1463 1963 +500 - Misses 746 985 +239 ``` | [Impacted Files](https://codecov.io/gh/datalad/datalad-gooey/pull/282?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) | Coverage Δ | | |---|---|---| | [datalad\_gooey/app.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9hcHAucHk=) | `70.56% <93.93%> (+6.06%)` | :arrow_up: | | [datalad\_gooey/utils.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS91dGlscy5weQ==) | `57.57% <100.00%> (+4.24%)` | :arrow_up: | | [datalad\_gooey/active\_suite.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9hY3RpdmVfc3VpdGUucHk=) | `75.00% <0.00%> (-4.17%)` | :arrow_down: | | [datalad\_gooey/param\_multival\_widget.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9wYXJhbV9tdWx0aXZhbF93aWRnZXQucHk=) | `75.00% <0.00%> (-1.80%)` | :arrow_down: | | [datalad\_gooey/simplified\_api.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9zaW1wbGlmaWVkX2FwaS5weQ==) | `100.00% <0.00%> (ø)` | | | [datalad\_gooey/tests/test\_param\_widget.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS90ZXN0cy90ZXN0X3BhcmFtX3dpZGdldC5weQ==) | `100.00% <0.00%> (ø)` | | | [datalad\_gooey/param\_alt\_widget.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9wYXJhbV9hbHRfd2lkZ2V0LnB5) | `87.87% <0.00%> (ø)` | | | [datalad\_gooey/param\_mixin.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9wYXJhbV9taXhpbi5weQ==) | `76.81% <0.00%> (ø)` | | | [datalad\_gooey/param\_path\_widget.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9wYXJhbV9wYXRoX3dpZGdldC5weQ==) | `56.86% <0.00%> (ø)` | | | [datalad\_gooey/param\_form\_utils.py](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9nb29leS9wYXJhbV9mb3JtX3V0aWxzLnB5) | `87.03% <0.00%> (+0.30%)` | :arrow_up: | | ... and [4 more](https://codecov.io/gh/datalad/datalad-gooey/pull/282/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

mih commented 1 year ago

After having used this for a while now, I am thinking of reverting this PR.

Two reasons:

Geometry changes of the main window do not save reliably. I am left with some "not-the-last" configuration for the outer window, although the header geometry in the browser widget seems to save properly every time
The UI file became uneditable in the designer. The main window layout is unrecognized (showing menubar and statusbar on top of each other), and any attempt to save an adjusted UI file leads to a runtime error on startup:

Designer: Attempt to add a layout to a widget 'MainWindow' (GooeyQMainWindow) which already has a layout of non-box type QMainWindowLayout.
This indicates an inconsistency in the ui-file.
[ERROR  ] Could not locate widget cmdTab (QWidget) (RuntimeError)

The latter can be circumvented by manually and temporarilly patching the UI file with this

diff --git a/datalad_gooey/resources/ui/main_window.ui b/datalad_gooey/resources/ui/main_window.ui
index d20e388..0a8986b 100644
--- a/datalad_gooey/resources/ui/main_window.ui
+++ b/datalad_gooey/resources/ui/main_window.ui
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <ui version="4.0">
  <class>MainWindow</class>
- <widget class="GooeyQMainWindow" name="MainWindow">
+ <widget class="QMainWindow" name="MainWindow">
   <property name="geometry">
    <rect>
     <x>0</x>

It should be possible to avoid subclassing QMainWindow for the purpose of running a custom close event implementation. Here is the sketch:

https://stackoverflow.com/a/52747646/1194609

datalad / datalad-gooey

Store/restore "main" geometry/state - window and the file tree header #282

Codecov Report