-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
silx.view.Viewer : New feature to display multiple curves in a new window #4132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I left a few suggestions.
There is an issue with test with PySide6, I guess None
is not accepted byt QStandardItem.appendRow
:
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/silx/app/view/test/test_view.py:89:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/silx/app/view/Viewer.py:120: in __init__
self._customPlotSelectionWindow = CustomPlotSelectionWindow(self)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/silx/app/view/CustomPlotSelectionWindow.py:460: in __init__
model = _FileListModel(self.plot1D)
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/silx/app/view/CustomPlotSelectionWindow.py:118: in __init__
self._addYFile("")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <silx.app.view.CustomPlotSelectionWindow._FileListModel(0x5585b7632640) at 0x7f4d1daa2480>
filename = '', curve = None, node = 'X'
def _addYFile(self, filename, curve=None, node="X"):
fileItem, iconItem, removeItem = self._createRowItems(filename, curve)
if not filename:
fileItem.setData(qt.QSize(0, 30), qt.Qt.SizeHintRole)
if self.getYParent().rowCount() == 0:
> self.getYParent().appendRow([None, fileItem])
E TypeError: 'PySide6.QtGui.QStandardItem.appendRow' called with wrong argument types:
E PySide6.QtGui.QStandardItem.appendRow(list)
E Supported signatures:
E PySide6.QtGui.QStandardItem.appendRow(PySide6.QtGui.QStandardItem)
E PySide6.QtGui.QStandardItem.appendRow(Sequence[PySide6.QtGui.QStandardItem])
Co-authored-by: Thomas VINCENT <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR 💪
Minor comments.
If we want to be picky a couple of lines could be added to the PR to explain how to use it:
- open silx view
- select a file
- open the 'plot selection' window (Views -> plot selection)
- drag and drop dataset to the new window
- admire the results :)
But I think the documentation part will come in another PR right ?.
Else nice user experience 😄
painter.save() | ||
painter.setPen(self.__highlightDropPen) | ||
painter.drawRect(option.rect.adjusted(1, 1, -1, -1)) | ||
painter.restore() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a 'painter' context would be more pythonic. Something which handle the save
and the restore
function
with painter_state_context(painter): # not very inspired by the name
painter.setPen(self.__highlightDropPen)
painter.drawRect(option.rect.adjusted(1, 1, -1, -1))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about it, and I'm not used to using "with", so for the moment I'll leave it at that.
|
||
def _addYFile(self, filename, curve=None, node="X"): | ||
fileItem, iconItem, removeItem = self._createRowItems(filename, curve) | ||
if not filename: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition was not 'straightforward' for me (maybe I am tired).
Anyway to help understanding here are options:
- add typing to filename.
because from outside for this condition, according to the context we can consider filename to be a string or None. If you provide
def _addYFile(self, filename: str, curve=None, node="X"):
Then this will allow us to remove one use case
2. provide a simple comment
if not filename:
# provide size hint for 'empty' item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, typing and docString will be added to the next PR.
A new feature has been added, allowing the display of multiple curves by selecting values for X or Y. Only 1D files are taken into account.
How to access :
All the code is in silx.view.CustomPlotSelectionWindow
![image](https://private-user-images.githubusercontent.com/102356226/339290683-52958037-c9c3-4d28-8353-6a8b6c98ea3b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkyNDMyNjgsIm5iZiI6MTczOTI0Mjk2OCwicGF0aCI6Ii8xMDIzNTYyMjYvMzM5MjkwNjgzLTUyOTU4MDM3LWM5YzMtNGQyOC04MzUzLTZhOGI2Yzk4ZWEzYi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjExJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxMVQwMzAyNDhaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0yODc0MzE3NTE4ZDFmNTg5YzhiMDBmMmRhZDUyZTA4OWYyYTZmOTg4NTk1ODE2MDBjNGE0NTU4NGE1MjhhOTI2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.50NHeBcYjeL_Ilg2KqIs_sVvuPi-B0hkWVG3Vk0uEos)