Add CloseEvent to DockItem #303

bburan · 2018-08-20T18:40:57Z

This will allow DockItem widgets to perform an action in response to the titlebar close button being clicked (and ignore this event if desired).

MatthieuDartiailh

LGTM

I will test later. @sccolbert an opinion on this ?

codecov-io · 2018-08-20T19:23:41Z

Codecov Report

Merging #303 into master will increase coverage by 0.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   70.21%   70.23%   +0.01%     
==========================================
  Files         304      305       +1     
  Lines       23625    23642      +17     
==========================================
+ Hits        16589    16604      +15     
- Misses       7036     7038       +2

Impacted Files	Coverage Δ
enaml/widgets/window.py	`58.04% <100%> (-1.3%)`	⬇️
enaml/widgets/dock_item.py	`84% <100%> (+0.66%)`	⬆️
enaml/qt/qt_dialog.py	`75.55% <100%> (ø)`	⬆️
enaml/qt/qt_window.py	`65.21% <100%> (+0.25%)`	⬆️
enaml/qt/qt_main_window.py	`60.68% <100%> (ø)`	⬆️
enaml/qt/qt_dock_item.py	`77.67% <85.71%> (+0.67%)`	⬆️
enaml/widgets/close_event.py	`88.88% <88.88%> (ø)`

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 1e5cd02...fcd42e6. Read the comment docs.

sccolbert · 2018-08-20T19:26:56Z

Fine with the concept. Not a fan of the circular _proxy_ref thing. There might be a cleaner way to express that, like using an event filter on the qwidget. I'm at jupytercon this week, so let's leave this open for a bit until I can take a closer look.

…

On Mon, Aug 20, 2018, 14:23 Codecov ***@***.***> wrote: Codecov <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=h1> Report Merging #303 <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=desc> into master <https://codecov.io/gh/nucleic/enaml/commit/1e5cd021ea54d9ca5fde8acecb415e2bd224fd94?src=pr&el=desc> will *increase* coverage by <.01%. The diff coverage is 87.5%. [image: Impacted file tree graph] <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=tree> @@ Coverage Diff @@## master #303 +/- ## ==========================================+ Coverage 70.21% 70.22% +<.01% ========================================== Files 304 305 +1 Lines 23625 23638 +13 ==========================================+ Hits 16589 16600 +11 - Misses 7036 7038 +2 Impacted Files <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=tree> Coverage Δ enaml/qt/qt_dialog.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvcXQvcXRfZGlhbG9nLnB5> 75.55% <100%> (ø) ⬆️ enaml/qt/qt_window.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvcXQvcXRfd2luZG93LnB5> 65.21% <100%> (+0.25%) ⬆️ enaml/widgets/dock_item.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvd2lkZ2V0cy9kb2NrX2l0ZW0ucHk=> 84% <100%> (+0.66%) ⬆️ enaml/widgets/window.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvd2lkZ2V0cy93aW5kb3cucHk=> 58.04% <100%> (-1.3%) ⬇️ enaml/qt/qt_main_window.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvcXQvcXRfbWFpbl93aW5kb3cucHk=> 60.68% <100%> (ø) ⬆️ enaml/qt/qt_dock_item.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvcXQvcXRfZG9ja19pdGVtLnB5> 76.85% <75%> (-0.15%) ⬇️ enaml/widgets/close_event.py <https://codecov.io/gh/nucleic/enaml/pull/303/diff?src=pr&el=tree#diff-ZW5hbWwvd2lkZ2V0cy9jbG9zZV9ldmVudC5weQ==> 88.88% <88.88%> (ø) ------------------------------ Continue to review full report at Codecov <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=continue>. *Legend* - Click here to learn more <https://docs.codecov.io/docs/codecov-delta> Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=footer>. Last update 1e5cd02...eae565d <https://codecov.io/gh/nucleic/enaml/pull/303?src=pr&el=lastupdated>. Read the comment docs <https://docs.codecov.io/docs/pull-request-comments> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIYScZaKyRrPYl0KhRwdW0z2QGZLsEYks5uSwy9gaJpZM4WEgm5> .

MatthieuDartiailh · 2018-08-20T19:34:50Z

Sure @sccolbert . The proxy_ref trick is similar to what you did on the QtWindow (but not at the same place and you used an atomref), hence why it was not really bothering to me (but I will have to check what atomref does...).

sccolbert · 2018-08-20T19:37:22Z

atomref is a more efficient weakref for atom objects

…

On Mon, Aug 20, 2018, 14:34 Matthieu Dartiailh ***@***.***> wrote: Sure @sccolbert <https://github.com/sccolbert> . The proxy_ref trick is similar to what you did on the QtWindow (but not at the same place and you used an atomref), hence why it was not really bothering to me (but I will have to check what atomref does...). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIYSbYh5FQvXxJD-Bb4F6ZJzFZ-jbSqks5uSw9agaJpZM4WEgm5> .

bburan · 2018-08-20T20:12:30Z

Are you suggesting:

class DockItemEventFilter(QObject):

    def __init__(self, declaration, *args, **kwargs):
        self.declaration = declaration
        QObject.__init__(self, *args, **kwargs)

    def eventFilter(self, object, event):
        if event.type() == QEvent.Close:
            close_event = CloseEvent()
            self.declaration.closing(close_event)
            if not close_event.is_accepted():
                event.ignore()
                return True
        return QObject.eventFilter(self, object, event)

Then doing the following in QtDockItem:

    def create_widget(self):
        """ Create the underlying QDockItem widget.

        """
        self.widget = QCustomDockItem(self.parent_widget())
        self.event_filter = DockItemEventFilter(self.declaration)
        self.widget.installEventFilter(self.event_filter)

sccolbert · 2018-08-20T20:39:04Z

Yes. However it might be worth looking at atomref if that's what I did for the Window class.

…

On Mon, Aug 20, 2018, 15:12 Brad Buran ***@***.***> wrote: Are you suggesting: class DockItemEventFilter(QObject): def __init__(self, declaration, *args, **kwargs): self.declaration = declaration QObject.__init__(self, *args, **kwargs) def eventFilter(self, object, event): if event.type() == QEvent.Close: close_event = CloseEvent() self.declaration.closing(close_event) if not close_event.is_accepted(): event.ignore() return True return QObject.eventFilter(self, object, event) Then doing the following in QtDockItem: def create_widget(self): """ Create the underlying QDockItem widget. """ self.widget = QCustomDockItem(self.parent_widget()) self.event_filter = DockItemEventFilter(self.declaration) self.widget.installEventFilter(self.event_filter) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIYSQFiPxNYvRwa4XP5fsUUBToVbS1pks5uSxgugaJpZM4WEgm5> .

bburan · 2018-08-20T22:23:48Z

Ok, sounds good. It now parallels what's done in the main window class (with atomref).

MatthieuDartiailh · 2018-08-28T03:06:12Z

ping @sccolbert

The implementation now matches the one used for windows. To me this is good to go. Let me know If you think it is worth re-visiting the whole idea using event filters.

sccolbert

Code looks good. Couple minor cleanup fixes needed before merging.

sccolbert · 2018-08-28T14:03:11Z

enaml/qt/qt_window.py

@@ -10,7 +10,8 @@
 from atom.api import Typed, atomref

 from enaml.layout.geometry import Pos, Rect, Size
-from enaml.widgets.window import ProxyWindow, CloseEvent
+from enaml.widgets.window import ProxyWindow
+from enaml.widgets.close_event import CloseEvent


close_event should be imported before window to maintain sort order. /pedantry

sccolbert · 2018-08-28T14:03:34Z

enaml/widgets/close_event.py

@@ -0,0 +1,35 @@
+from atom.api import Atom, Bool


This file needs a copyright header.

This is always a bit unclear to me but can the copyright mention 2013 when the file was created only in 2018 ?

sccolbert · 2018-08-28T14:04:32Z

enaml/widgets/window.py

@@ -15,6 +15,7 @@
 from enaml.layout.geometry import Pos, Rect, Size

 from .container import Container
+from .close_event import CloseEvent


This should be imported before .container to maintain sort order.

sccolbert · 2018-08-28T18:07:04Z

ideally, all files should be updated to current year.

…

On Tue, Aug 28, 2018 at 1:04 PM, Matthieu Dartiailh < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In enaml/widgets/close_event.py <#303 (comment)>: > @@ -0,0 +1,35 @@ +from atom.api import Atom, Bool This is always a bit unclear to me but can the copyright mention 2013 when the file was created only in 2018 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#303 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIYSTJ-v3YrABbCkwz-NSE43RfRUiw5ks5uVYZHgaJpZM4WEgm5> .

sccolbert · 2018-08-28T18:07:55Z

some of the imports in this PR are still in the wrong order

…

On Tue, Aug 28, 2018 at 1:07 PM, Chris Colbert ***@***.***> wrote: ideally, all files should be updated to current year. On Tue, Aug 28, 2018 at 1:04 PM, Matthieu Dartiailh < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In enaml/widgets/close_event.py > <#303 (comment)>: > > > @@ -0,0 +1,35 @@ > +from atom.api import Atom, Bool > > This is always a bit unclear to me but can the copyright mention 2013 > when the file was created only in 2018 ? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#303 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAIYSTJ-v3YrABbCkwz-NSE43RfRUiw5ks5uVYZHgaJpZM4WEgm5> > . >

sccolbert · 2018-08-28T19:46:42Z

enaml/qt/qt_dialog.py

@@ -8,7 +8,7 @@
 from atom.api import Typed, atomref

 from enaml.widgets.dialog import ProxyDialog
-from enaml.widgets.window import CloseEvent
+from enaml.widgets.close_event import CloseEvent


Almost there. This is the last one that's out of order.

sccolbert · 2018-08-28T21:02:07Z

MatthieuDartiailh · 2018-08-29T13:43:50Z

Thanks @bburan this is a welcome addition. Thanks @sccolbert for the reviews.

bburan · 2018-09-04T20:52:06Z

@MatthieuDartiailh My colleague was trying to test out Enaml by cutting-and-pasting examples from readthedocs. He couldn't get the dock_area.enaml to work because I added a closing event to the DockItem (to demo the addition in this PR) and the current conda channels do not yet reflect master (but readthedocs.org does). Do we want to worry about this detail? My coworker was getting a bit frustrated testing it out before he contacted me.

MatthieuDartiailh · 2018-09-04T21:34:51Z

I probably should compile the docs for released versions (never had time to set that up). And make sure people notice they are looking at the development version docs. Thanks for reporting.

MatthieuDartiailh · 2018-09-06T03:00:53Z

Sadly for the last release the docs are broken ... I will add doc building to the CI and hopefully starting with the next release the stable docs will be available.

Add CloseEvent to DockItem

eae565d

This will allow DockItem widgets to perform an action in response to the titlebar close button being clicked (and ignore this event if desired).

MatthieuDartiailh approved these changes Aug 20, 2018

View reviewed changes

Use atomref

3b4f312

sccolbert requested changes Aug 28, 2018

View reviewed changes

Fix sort order of imports and copyright headers

932138f

Fix ordering of imports

61ae494

sccolbert reviewed Aug 28, 2018

View reviewed changes

Fix import order

fcd42e6

sccolbert approved these changes Aug 28, 2018

View reviewed changes

MatthieuDartiailh merged commit 26544b0 into nucleic:master Aug 29, 2018

bburan deleted the bburan/dock-item-closing branch September 6, 2018 18:15

MatthieuDartiailh mentioned this pull request Sep 9, 2018

Better handling of attempt to close a measure edition panel Exopy/exopy#154

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CloseEvent to DockItem #303

Add CloseEvent to DockItem #303

bburan commented Aug 20, 2018

MatthieuDartiailh left a comment

codecov-io commented Aug 20, 2018 •

edited

Loading

sccolbert commented Aug 20, 2018 via email

MatthieuDartiailh commented Aug 20, 2018

sccolbert commented Aug 20, 2018 via email

bburan commented Aug 20, 2018

sccolbert commented Aug 20, 2018 via email

bburan commented Aug 20, 2018

MatthieuDartiailh commented Aug 28, 2018

sccolbert left a comment

sccolbert Aug 28, 2018

sccolbert Aug 28, 2018

MatthieuDartiailh Aug 28, 2018

sccolbert Aug 28, 2018

sccolbert commented Aug 28, 2018 via email

sccolbert commented Aug 28, 2018 via email

sccolbert Aug 28, 2018

sccolbert commented Aug 28, 2018

MatthieuDartiailh commented Aug 29, 2018

bburan commented Sep 4, 2018

MatthieuDartiailh commented Sep 4, 2018

MatthieuDartiailh commented Sep 6, 2018

Add CloseEvent to DockItem #303

Add CloseEvent to DockItem #303

Conversation

bburan commented Aug 20, 2018

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 20, 2018 • edited Loading

Codecov Report

sccolbert commented Aug 20, 2018 via email

MatthieuDartiailh commented Aug 20, 2018

sccolbert commented Aug 20, 2018 via email

bburan commented Aug 20, 2018

sccolbert commented Aug 20, 2018 via email

bburan commented Aug 20, 2018

MatthieuDartiailh commented Aug 28, 2018

sccolbert left a comment

Choose a reason for hiding this comment

sccolbert Aug 28, 2018

Choose a reason for hiding this comment

sccolbert Aug 28, 2018

Choose a reason for hiding this comment

MatthieuDartiailh Aug 28, 2018

Choose a reason for hiding this comment

sccolbert Aug 28, 2018

Choose a reason for hiding this comment

sccolbert commented Aug 28, 2018 via email

sccolbert commented Aug 28, 2018 via email

sccolbert Aug 28, 2018

Choose a reason for hiding this comment

sccolbert commented Aug 28, 2018

MatthieuDartiailh commented Aug 29, 2018

bburan commented Sep 4, 2018

MatthieuDartiailh commented Sep 4, 2018

MatthieuDartiailh commented Sep 6, 2018

codecov-io commented Aug 20, 2018 •

edited

Loading