FIX: problem when closing settings fig #85

juangpc · 2022-03-31T17:16:01Z

Hi,

This PR attempts to fix #78 (opened by @jasmainak).

There are two small problems/typos and a QEvent object which is not sent to the closeEvent call.

agramfort · 2022-04-03T13:20:58Z

I confirm it fixes the problem. Any chance we could add a test for this?

thx @juangpc !

juangpc · 2022-04-04T11:17:42Z

Thank you.
Yes, I'll see what I can do to test this menu interaction.

juangpc · 2022-04-05T15:45:56Z

Hello again,

Following @agramfort comment, I'm trying to provide additional test coverage.

The tests I've added would've shattered the original issue (#78) so that is progress. However there are some comments, questions I'd like to share:

I'm trying to keep all the structure of the code as I see it now. I see that functionality of the mne-qt-browser is now being tested in the test_raw.py script inside the mne-python repo. Although some helpers functions are added as members to the class PyQtGraphBrowser which is in _py_figure.py inside the mne-qt-browser repo.
Even in this repository documentation, when explaining how to do the tests, one needs to go to run them to the mne-python repository. So I'm trying to follow this structure.
There are thus two linked necessary PRs. One being this one, and a second one on the mne-python repo. I haven't created it yet. I'll wait to see what is best. It will come from this branch on my fork. Should I create it?
I've added the mark pgtest so that it doesn't interact with the matplotlib related classes. Is this correct?
I've added the QTest dependency for the test_raw.py script in mne-python repo. Is this ok? It works in my env.
Added additional tests to cover all the members (QActions) on the toolbar of the mne-qt-browser. Should I create a separate PR?
None of my branches are rebased. Should I? I hear you guys rebase and later squash. Checked the documentation and contribution guide I couldn't find anything related.
This PR, and any later one, will probably be marked as "Allow edits by maintainers". Please, feel free to edit the code in my fork. No problem at all.

Thanks.

agramfort · 2022-04-05T19:29:00Z

@juangpc can you link the PR here I don't see it?

there are some tests run here. Any chance to add a test here?

juangpc · 2022-04-07T05:20:09Z

Hi,

Thanks for your mention @hoechenberger.

I (think I) have followed @agramfort guidance to place here all the tests. I've increased the intensity and granularity with which the GUI components are tested. My intention is to eventually lead to a 100% coverage.
There is this sensitivity slider in the settings dialog issue I've created (Sensitivity slider in Settings window #103). I don't know exactly what it is supposed to do, but it doesn't seem to be doing too much. The effect is similar to widening the shown time span. In any case, the test, does test the slider as a GUI component, and its effect in the model. This component is neighboring the Settings dialog, which is the original subject of this PR. I think it can belong to the same PR.
I've added a ',' comma shortcut for the settings dialog. It was quite useful during development and I figure it might become useful for the user. Many SW applications use this shortcut for settings. (Again) feel free to delete this shortcut. No hurt feelings.
Changed the order of the subsampling methods because 'mean' seems more convenient as a default method, (index 0).
Since the tests are now migrating towards this repository, I've edited the README file too.

Thanks,

juangpc · 2022-04-07T05:21:41Z

Ready for merge on my side.

hoechenberger · 2022-04-07T05:36:14Z

Hello @juangpc, thanks for the effort you're putting in here!!

I'm just a little concerned this work is getting a bit outside of the scope of the original idea of this PR, and I wonder if it would make sense to split it into smaller, separate PRs that each basically do one thing only. This will make the review process faster and easier, and allows for a better discussion of the individual changes you're proposing!

For example, while I do like the idea of adding a shortcut for settings, it should conform to the system standards, e.g. Cmd+, on macOS, and to whatever the defaults might be on Windows and Linux.

juangpc · 2022-04-07T05:38:16Z

Ok. No worries. I’ll drop that. Thanks anyway.

On Thu, Apr 7, 2022 at 1:36 AM Richard Höchenberger < ***@***.***> wrote: Hello @juangpc <https://github.com/juangpc>, thanks for the effort you're putting in here!! I'm just a little concerned this work is getting a bit outside of the scope of the original idea of this PR, and I wonder if it would make sense to split it into smaller, separate PRs that each basically do one thing only. This will make the review process faster and easier, and allows for a better discussion of the individual changes you're proposing! For example, while I do like the idea of adding a shortcut for settings, it should conform to the system standards, e.g. Cmd+, on macOS, and to whatever the defaults might be in windows and Linux. — Reply to this email directly, view it on GitHub <#85 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEKMIG63SOLI7GMPT37RZTVDZX5TANCNFSM5SGB4NTQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

--

…

-- Juan

agramfort · 2022-04-07T17:22:49Z

mne_qt_browser/tests/test_pg_specific.py

 import numpy as np
+import math


Suggested change

import numpy as np

import math

import math

import numpy as np

import standard lib first

with later changes math lib no longer needed.

mne_qt_browser/tests/test_pg_specific.py

hoechenberger · 2022-04-07T19:45:41Z

mne_qt_browser/_pg_figure.py

@@ -1697,10 +1697,10 @@ def __init__(self, main, **kwargs):
 class ProjDialog(_BaseDialog):
    """A dialog to toggle projections."""

-    def __init__(self, main, *, name):
+    def __init__(self, main, title='Projectors', **kwargs):


Why do we need the flexibility to adjust the title of that dialog?

Also, in #101 I specifically changed the signature to not accept kwargs and instead enforce passing keyword-only arguments to ensure better code health. Please revert your change to this line

this one also needs to be reverted. We really try to avoid kwargs in mne as it makes the code super hard to document and test.

I spent some time looking at the code with @juangpc and I agree **kwargs made the code really hard to read. And it's nested really deep with the backend handling. What used to be "params" in the past is now "kwargs"

I see. Well. I'm sorry. I'll fix this.

Why do we need the flexibility to adjust the title of that dialog?

I don't need it. I changed it back because: 1. coherence. 2. your code when improperly merged with mine, introduced a bug.
Attending 1) there are 5 or so dialog windows child classes to _BaseDialog. And the way the child classes communicate with it in the other instances is different from what you proposed in your change. See SettingsDialog or HelpDialog, all go in the same toolbar.
Attending 2) The merge with your change introduced a bug in my fork. It might have been me or automatic merge. Don't remember, it was late. This has been solved now. I was surprised by your change, the tests found it. The tests I push here, actually found the bug. So I guess thats a good thing. Anyway it was a bad merge. I'll fix this.

Also, I get a warning when using name as argument in line 1700: "Shadows name 'name' from greater scope".

I don't know the difference between single star "*" and "**kwargs". This is my first python pr.

Attended: 0408af1

hoechenberger · 2022-04-07T19:47:11Z

mne_qt_browser/_pg_figure.py

        self.external_change = True
        # Create projection-layout
-        super().__init__(main.window(), name=name, title='Projectors')
+        super().__init__(main.window(), title=title, **kwargs)


See above, please revert this

Attended 0408af1

agramfort · 2022-04-07T19:51:52Z

mne_qt_browser/_pg_figure.py

@@ -1697,10 +1697,10 @@ def __init__(self, main, **kwargs):
 class ProjDialog(_BaseDialog):
    """A dialog to toggle projections."""

-    def __init__(self, main, *, name):
+    def __init__(self, main, title='Projectors', **kwargs):


@juangpc you need to be careful when making large changes including to public functions. One PR should be on one feature / one bug.

Why do you need these changes to address the fix aimed at here? If it's not related to the fix can you revert the unrelated changes and put this in a follow up PR that can be discussed with the other devs?

Why do you need these changes to address the fix aimed at here?

I don't. Attended here: 0408af1

agramfort · 2022-04-07T20:06:11Z

mne_qt_browser/_pg_figure.py

@@ -1578,7 +1578,7 @@ def __init__(self, main, title='Settings', **kwargs):
                '<i>(Those methods are adapted from '
                'pyqtgraph)</i><br>'
                'Default is "peak".')
-        self.ds_method_cmbx.addItems(['subsample', 'mean', 'peak'])
+        self.ds_method_cmbx.addItems(['mean', 'subsample', 'peak'])


this is unrelated right?

clearly unrelated to the crash. Attended here: 798c39d

juangpc · 2022-04-07T20:53:40Z

Ready on my side.

larsoner

Other than a couple of small suggestions, LGTM!

mne_qt_browser/tests/test_pg_specific.py

README.md

juangpc · 2022-04-08T14:10:16Z

Ready on my side.

Since this is getting close to ready for merging. Let me thank you all for your time. Hope this is useful.

agramfort · 2022-04-08T14:23:29Z

@larsoner I let you merge if happy

larsoner · 2022-04-08T14:48:33Z

Thanks @juangpc !

fix typo in _toggle_help_fig

6c40f7f

juangpc changed the title ~~fix typo in _toggle_help_fig~~ FIX: problem when closing settings fig Apr 1, 2022

define _fake_click_on_toolbar_action member fcn in PyQtGraphBrowser

8865cd0

juangpc added 3 commits April 5, 2022 13:08

add window title to settings and help windows dialogs

3da8095

change order of downsampling methods in settings window

89362d4

add comma as keyboard shortcut for settings window

4f69bce

hoechenberger mentioned this pull request Apr 6, 2022

Always keep projectors window on top #101

Merged

juangpc added 2 commits April 6, 2022 23:53

add extra tests for other toolbar menus

e6462cb

fix conflict Settings window title

0edb436

juangpc force-pushed the fix_settings_menu_crash branch from 19e097d to 0edb436 Compare April 7, 2022 04:04

juangpc added 2 commits April 7, 2022 00:05

fix conflict Help window title

069a97a

add projections window title

c890cc8

juangpc mentioned this pull request Apr 7, 2022

Sensitivity slider in Settings window #103

Closed

juangpc added 6 commits April 7, 2022 00:40

update readme markup instructions in repo

d1757a1

add floating point accuracy

f7d8d57

improve test toolbox time span shown

e73c698

improve test toolbar channel number shown

7241b84

fix problem in test toolbar time span shown

1ebfdaa

fix merge conflict in projector dialog window title

5341a2b

juangpc added 3 commits April 7, 2022 10:57

fix projection input arg forwarded to parent class

70a5a81

delete comma shortcut for settings dialog

01bdf10

set fig_projections to none after deleting dialog

1420f85

Merge branch 'main' into fix_settings_menu_crash

8464bc2

agramfort reviewed Apr 7, 2022

View reviewed changes

juangpc added 7 commits April 7, 2022 13:42

improve README markdown file for repo

61239ce

minor edit typo in comment

e855ffd

fix argument forwarding to superclass projections dialog

e77d266

fix typo comment kwargs

6375588

delete fullscreen toolbar action test, no longer there

ef7569a

fix change in name of toolbar actions in test

d80e5ac

import standard lib first

e6ec944

hoechenberger reviewed Apr 7, 2022

View reviewed changes

agramfort reviewed Apr 7, 2022

View reviewed changes

make compliant with flake8

3aefc9d

agramfort reviewed Apr 7, 2022

View reviewed changes

juangpc added 2 commits April 7, 2022 16:13

revert change order of subsampling methods

798c39d

revert change in ProjDialog constructor

0408af1

larsoner reviewed Apr 7, 2022

View reviewed changes

mne_qt_browser/tests/test_pg_specific.py Outdated Show resolved Hide resolved

mne_qt_browser/tests/test_pg_specific.py Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

juangpc added 3 commits April 8, 2022 08:56

select benchmark funcs with mark not file

15c4813

improve scroll control test

e8d91bb

use explicit length sensitivity tests. flake compliance

4cb3573

agramfort approved these changes Apr 8, 2022

View reviewed changes

larsoner merged commit 3d3b199 into mne-tools:main Apr 8, 2022

juangpc deleted the fix_settings_menu_crash branch April 8, 2022 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: problem when closing settings fig #85

FIX: problem when closing settings fig #85

juangpc commented Mar 31, 2022

agramfort commented Apr 3, 2022

juangpc commented Apr 4, 2022

juangpc commented Apr 5, 2022 •

edited

Loading

agramfort commented Apr 5, 2022

juangpc commented Apr 7, 2022

juangpc commented Apr 7, 2022

hoechenberger commented Apr 7, 2022 •

edited

Loading

juangpc commented Apr 7, 2022 via email

agramfort Apr 7, 2022

juangpc Apr 7, 2022

hoechenberger Apr 7, 2022 •

edited

Loading

agramfort Apr 7, 2022

jasmainak Apr 7, 2022

juangpc Apr 7, 2022

juangpc Apr 7, 2022

hoechenberger Apr 7, 2022

juangpc Apr 7, 2022

agramfort Apr 7, 2022

juangpc Apr 7, 2022

agramfort Apr 7, 2022

juangpc Apr 7, 2022

juangpc commented Apr 7, 2022

larsoner left a comment

juangpc commented Apr 8, 2022

agramfort commented Apr 8, 2022

larsoner commented Apr 8, 2022

FIX: problem when closing settings fig #85

FIX: problem when closing settings fig #85

Conversation

juangpc commented Mar 31, 2022

agramfort commented Apr 3, 2022

juangpc commented Apr 4, 2022

juangpc commented Apr 5, 2022 • edited Loading

agramfort commented Apr 5, 2022

juangpc commented Apr 7, 2022

juangpc commented Apr 7, 2022

hoechenberger commented Apr 7, 2022 • edited Loading

juangpc commented Apr 7, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juangpc commented Apr 7, 2022

larsoner left a comment

Choose a reason for hiding this comment

juangpc commented Apr 8, 2022

agramfort commented Apr 8, 2022

larsoner commented Apr 8, 2022

juangpc commented Apr 5, 2022 •

edited

Loading

hoechenberger commented Apr 7, 2022 •

edited

Loading

hoechenberger Apr 7, 2022 •

edited

Loading