Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

app refactored as stand-alone widget #4

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sergirubio
Copy link

to integrate it into another applications, it must be an independent widget
I refactored the code of tpgarchiving into tpgarchivingwidget
original file not modified

Copy link
Member

@cpascual cpascual left a comment

Choose a reason for hiding this comment

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

I checked the PR code without running it... I'll write my comments inline

Comment on lines +91 to +100
def override_mousePressEvent(widget, button, callback):
old_hook = widget.mousePressEvent
def mousePressEvent(obj, event):
if event.button() == button:
callback(obj, event)
old_hook(event)
widget.mousePressEvent = types.MethodType(mousePressEvent, widget)

override_mousePressEvent(self.ui.listView,
Qt.Qt.RightButton, self.onContextMenu)
Copy link
Member

Choose a reason for hiding this comment

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

This part looks like quite a hack... I would try to avoid intercepting mouse press events (and events in general) and use higher level Qt APIs instead.
...and if working with events is absolutely mandatory, I would consider using an event filter

Comment on lines +38 to +40
import fandango
import fandango as fn
import fandango.qt
Copy link
Member

Choose a reason for hiding this comment

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

I would not introduce fandango as a dependency. On a quick look it seems that some of these imports are not even used, and others are quite trivial utility helpers that could be easily replaced

(I mark it here, but there are other similar imports in the other files)

@@ -0,0 +1,181 @@
=====================================
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this file?

Is it the seed for some documentation or is it a collection of personal development notes?

#plot.setDropLogger(fandango.printf)
#plot.setSupportedMimeTypes(fandango.qt.MIMETYPES)

plot.setBackgroundBrush(Qt.QColor('white'))
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we should keep visual consistency among the various taurus* "oficial" projects (at least those under the taurus-org umbrella).

In that sense, I think that we should not be changing the background of the plots here, but intead do it either in taurus_pyqtgraph in general, or in a final application outside tango-archiving.

If we want to do it in general, we need to consider things with some care (that is why it hasn't been done yet)... but I think this is a time as good as any other to start the discussion, so I created this issue in taurus_pyqtgraph

Copy link
Member

Choose a reason for hiding this comment

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

the same comment is obviously valid for the other usages of setBackgroundBrush()

@sergirubio
Copy link
Author

Ok, do the changes you consider necessary, as long as it keeps the current functionality.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants