MRG: _TimeViewer resource guide #7606

GuillaumeFavelier · 2020-04-14T15:15:40Z

This PR adds a small README that describes the instructions to update the resource.py file used for _TimeViewer.

codecov · 2020-04-14T16:40:51Z

Codecov Report

Merging #7606 into master will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##           master    #7606   +/-   ##
=======================================
  Coverage   90.08%   90.09%           
=======================================
  Files         454      454           
  Lines       83551    83568   +17     
  Branches    13228    13238   +10     
=======================================
+ Hits        75266    75287   +21     
+ Misses       5420     5418    -2     
+ Partials     2865     2863    -2

mne/images/README.rst

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

GuillaumeFavelier · 2020-04-16T12:26:10Z

Azure seems to take a bit more time. I don't think it's related to this PR.

hoechenberger · 2020-04-16T13:06:02Z

Azure seems to take a bit more time. I don't think it's related to this PR.

I believe it's getting canceled bc the build is taking too long… anyway, LGTM.

larsoner · 2020-04-16T14:12:59Z

mne/images/README.rst

+Patching
+========
+
+The output file imports ``PyQt5`` globally, which is not consistent with MNE core


Rather than all of this, can you nest the import of resources.py wherever it's actually used in the MNE code?

I will try that right now

mne/images/README.rst

GuillaumeFavelier · 2020-04-16T15:24:35Z

Okay, I think I'll just keep everything in mne/images:

the SVG icons
the resource config file mne.qrc
the resource file itself resources.py

larsoner · 2020-04-16T15:43:25Z

That sounds like a nice solution to me

Also you have a conflict

GuillaumeFavelier · 2020-04-17T09:05:57Z

Waiting for the CIs to come back happy. Thanks to #7606 (comment) it should be way easier to update the icons.

hoechenberger · 2020-04-17T09:40:57Z

Okay, I think I'll just keep everything in mne/images:

Excellent

hoechenberger · 2020-04-17T09:42:23Z

btw are you planning do add "images" to that directory that would not be some kind of icon? Just asking bc if not, it could make sense to name the directory "icons" instead?

GuillaumeFavelier · 2020-04-17T09:50:32Z

You're right, icons is a better name.

GuillaumeFavelier · 2020-04-17T11:26:52Z

Rather than all of this, can you nest the import of resources.py wherever it's actually used in the MNE code?

@larsoner Travis fails because of this import during test collection:

mne-python/mne/icons/resources.py

Line 9 in 934d77f

from PyQt5 import QtCore

I tried importing it where it's needed in MNE (in _TimeViewer) but I don't think it's enough.

mne-python/mne/viz/_brain/_timeviewer.py

Lines 850 to 852 in 934d77f

    
           def load_icons(self): 
        
               from PyQt5.QtGui import QIcon 
        
               from ...icons import resources  # noqa

mne-python/mne/viz/_brain/_timeviewer.py

Lines 1095 to 1097 in 934d77f

    
           @safe_event 
        
           def clean(self): 
        
               from ...icons import resources

Do you have an idea?

larsoner · 2020-04-17T12:45:35Z

It will be imported during test collection unless you add it to the ignore list in setup.cfg or maybe mne/conftest.py, it's in one of those

larsoner · 2020-04-17T12:46:18Z

Ignore list, sorry mobile...

GuillaumeFavelier · 2020-04-17T12:47:23Z

Wow, thank you! I was not aware of such a system.

larsoner · 2020-04-17T13:48:29Z

mne/viz/_brain/_timeviewer.py

    def load_icons(self):
        from PyQt5.QtGui import QIcon
-        resources.qInitResources()
+        from ...icons import resources  # noqa


rather than a noqa and magic init-on-import, I'd leave it closer to how it was and do

from ...icons import resources resources.qInitResources()

BTW any problem with calling this multiple times (which will happen if you load multiple windows)? If so then a little helper init_resources that inits them exactly once could be helpful.

No problem at all, I tested it with link_brains().

GuillaumeFavelier · 2020-04-17T14:30:14Z

Oh I misunderstood: there is no problem calling init multiple times but I still did _init_resources.

larsoner

LGTM, please merge once CIs are happy

larsoner · 2020-04-17T14:32:41Z

Doing it once or multiple times is fine by me, up to you

GuillaumeFavelier · 2020-04-17T14:33:27Z

It works. I'll merge like this

GuillaumeFavelier added 2 commits April 14, 2020 17:11

Add a README for resource.py

4ec1c60

Fix typo

a171706

GuillaumeFavelier self-assigned this Apr 14, 2020

GuillaumeFavelier changed the title ~~_TimeViewer resource guide~~ MRG: _TimeViewer resource guide Apr 14, 2020

hoechenberger reviewed Apr 14, 2020

View reviewed changes

drammock reviewed Apr 14, 2020

View reviewed changes

mne/images/README.rst Outdated Show resolved Hide resolved

GuillaumeFavelier and others added 8 commits April 15, 2020 10:44

Update mne/images/README.rst

8c04128

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/images/README.rst

73cc43d

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/images/README.rst

a9c4222

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/images/README.rst

98b4334

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne/images/README.rst

49fc610

Co-Authored-By: Richard Höchenberger <richard.hoechenberger@gmail.com>

Use double backticks

2278bc1

Clarify PEP8 situation

23d2cca

Merge branch 'master' into time_viewer_resource_guide

37b8601

hoechenberger approved these changes Apr 16, 2020

View reviewed changes

larsoner reviewed Apr 16, 2020

View reviewed changes

mne/images/README.rst Outdated Show resolved Hide resolved

GuillaumeFavelier mentioned this pull request Apr 16, 2020

MRG: TimeViewer save movie #7612

Merged

GuillaumeFavelier changed the title ~~MRG: _TimeViewer resource guide~~ WIP: _TimeViewer resource guide Apr 16, 2020

GuillaumeFavelier added 3 commits April 16, 2020 16:51

Remove patching

c9bd52d

Ignore resources.py style

0af8896

Exclude from pydocstyle too

805b844

GuillaumeFavelier added 2 commits April 17, 2020 10:55

Merge branch 'master' into time_viewer_resource_guide

abc8bb0

Rework architecture

ac3fd16

GuillaumeFavelier added 3 commits April 17, 2020 11:54

Rename folder to icons

ba634a8

Fix pydocstyle exclude

52626fa

Fix MANIFEST exclude

934d77f

GuillaumeFavelier changed the title ~~WIP: _TimeViewer resource guide~~ MRG: _TimeViewer resource guide Apr 17, 2020

GuillaumeFavelier changed the title ~~MRG: _TimeViewer resource guide~~ WIP: _TimeViewer resource guide Apr 17, 2020

Fix pytest exclude

804a1fd

GuillaumeFavelier changed the title ~~WIP: _TimeViewer resource guide~~ MRG: _TimeViewer resource guide Apr 17, 2020

larsoner reviewed Apr 17, 2020

View reviewed changes

GuillaumeFavelier changed the title ~~MRG: _TimeViewer resource guide~~ WIP: _TimeViewer resource guide Apr 17, 2020

GuillaumeFavelier added 2 commits April 17, 2020 15:59

Use explicit qInitResources

a0065c0

Init resources only once

04f5129

larsoner approved these changes Apr 17, 2020

View reviewed changes

GuillaumeFavelier changed the title ~~WIP: _TimeViewer resource guide~~ MRG: _TimeViewer resource guide Apr 17, 2020

larsoner merged commit 90c5c8e into mne-tools:master Apr 19, 2020

GuillaumeFavelier deleted the time_viewer_resource_guide branch June 11, 2020 09:44

Uh oh!

MRG: _TimeViewer resource guide #7606

MRG: _TimeViewer resource guide #7606

Uh oh!

Conversation

GuillaumeFavelier commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeFavelier commented Apr 16, 2020

Uh oh!

hoechenberger commented Apr 16, 2020

Uh oh!

larsoner Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

GuillaumeFavelier Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeFavelier commented Apr 16, 2020

Uh oh!

larsoner commented Apr 16, 2020

Uh oh!

GuillaumeFavelier commented Apr 17, 2020

Uh oh!

hoechenberger commented Apr 17, 2020

Uh oh!

hoechenberger commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeFavelier commented Apr 17, 2020

Uh oh!

GuillaumeFavelier commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Apr 17, 2020

Uh oh!

GuillaumeFavelier commented Apr 17, 2020

Uh oh!

larsoner Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

GuillaumeFavelier Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

GuillaumeFavelier commented Apr 17, 2020

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Apr 17, 2020

Uh oh!

GuillaumeFavelier commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GuillaumeFavelier commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 14, 2020 •

edited

Loading

hoechenberger commented Apr 17, 2020 •

edited

Loading

GuillaumeFavelier commented Apr 17, 2020 •

edited

Loading

larsoner commented Apr 17, 2020 •

edited

Loading