GUI: add Diff-view page #112

tangkong · 2024-12-04T00:33:13Z

Description

Adds a Diff Page that subclasses the existing common views to add highlighting based on the calculated Diff
Adds a QtSingleton class that allows singletons to hold signals.
- Long story short, the normal __new__ singleton creation method doesn't quite work. While the singletons are created correctly, the bound QtCore.Signal's lose their connections every time an instance is returned
- Devan theorizes that this has to do with __init__ doing something to the class variable signals. I am too tired to investigate further

This description is so minimal for the amount of time I spent spinning on this 💀

This is likely still riddled with bugs, but I want to get eyes on it before it bloats into a small planet of a diff. I'm happy to revisit this in follow-up PRs, but I feel like there are other more pressing tasks to take on. (And I'm tired of thinking about diffs)

Outstanding tasks:

add comparison context menus to main window tree view (this overrides the common views, so I left it for now)
Touchups on diff highlighting. (I'm bad at logic-ing things)

Motivation and Context

#48 in part

How Has This Been Tested?

I added a smoke test. I should have probably added more

Where Has This Been Documented?

This PR. I'll go through for a docstring pass shortly

Context menus have been added to all non-main-window views

Highlighting helps, but could be more thorough / accurate

HIghlighting for entries with PV-entries

Pre-merge checklist

Code works interactively
Code follows the style guide
Code contains descriptive docstrings, including context and API
New/changed functions and methods are covered in the test suite where possible
Test suite passes locally
Test suite passes on GitHub Actions
Ran docs/pre-release-notes.sh and created a pre-release documentation page
Pre-release docs include context, functional descriptions, and contributors as appropriate

…er diff model subclasses

…ootTreeView model class specification

…inheritance and mixins

…o select entries for diff comparison from shared views

…mitted in models

tangkong · 2024-12-04T18:44:45Z

I'd like to request reviews here, focused mostly on the design/organization of the code. If you have the patience to QA the diff highlighting, I welcome that as well, but I do intend to circle back to that in the near future.

ZLLentz

I don't think I recommended any actions in my comments, it's all questions, so maybe I'm just a confused person. The code is clean so it was easy to focus on the small things.

ZLLentz · 2024-12-04T21:16:28Z

superscore/compare.py

@@ -49,6 +56,17 @@ def __repr__(self) -> str:

        return repr_str

+    @property
+    def type(self) -> DiffType:
+        if (self.original_value is not None) and (self.new_value is None):


Are there any data types in superscore that allow None as a valid value?

This is a good point, there are some fields in our dataclasses that allow None (optional fields). Though I don't think this changes how the DiffItem interprets values going from something -> None, this would still be a deletion as far as the diff types go. (or an addition for None-> something)

ZLLentz · 2024-12-04T21:17:57Z

superscore/compare.py

+        elif (self.original_value is not None) and (self.new_value is not None):
+            return DiffType.MODIFIED
+        else:
+            raise ValueError("DiffItem original and new values are both None")


This implies that DiffItem is only created when there is some modification, which I assume must be true. Is there any value in including a DiffType.NOT_MODIFIED or would this be unnecessary?

Currently this is true; DiffItems are only created when modifications are detected (or at least that's my intention). I imagine it's most concise to have only modifications show up, otherwise we'll have a DiffItem for every single field in every dataclass.

ZLLentz · 2024-12-04T21:21:01Z

superscore/tests/test_page.py

+            Setpoint(pv_name="MY:SET", data=1),
+            Readback(pv_name="MY:RBV", data=1),
+        ]
+    )
+    r_snapshot = Snapshot(
+        title="r_snap",
+        description="r desc",
+        children=[
+            Setpoint(pv_name="MY:SET", data=2),
+            Readback(pv_name="MY:RBV", data=1),
+            Readback(pv_name="MY:RBV2", data=2),


Is this test fixture sufficient? I haven't read on yet but I imagine that this includes ADDED and MODIFIED but skips over DELETED.

This is certainly not a sufficient test case. I definitely need to build out the tests, I was hoping to skate by this time and circle back to it.

ZLLentz · 2024-12-04T21:35:40Z

superscore/widgets/core.py

+    def __call__(cls, *args, **kwargs):
+        if cls._instance is None:
+            cls._instance = super().__call__(*args, **kwargs)
+        return cls._instance


I'm writing this out because I'm trying to build understanding.

My base-level understanding of what this does is:

When we use this as a metaclass, QtSingleton() (__init__) is used instead of type() to construct the subclass.

Then, the resulting class's constructor runs through __call__

The first time we create this class, it creates like normal and caches the result onto the class itself

The second time onward, the previously created class is returned instead

This is done in a way that makes qt happy in that each "instance" of the singleton is literally the same object so that the signals are literally the same objects and then things work as we'd hope

Yes, for the most part. The other way singletons may be created in python classes is with the __new__ dunder method, caching the first instance of the class in a similar way but with a different dunder method.

This effectively returned a singleton with each instantiation request, but also cleared signal connections along with it. I don't entirely understand why this happens, but I do know the metaclass approach with __call__ works.

A rabbit hole I'll jump down at some other point

I bet it has something to do with the precise way pyqt bridges the gap between python and the C objects for the purpose of qt signals...

superscore/widgets/page/diff.py

ZLLentz · 2024-12-04T23:47:29Z

superscore/widgets/page/diff.py

+    """
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.inverse = {}


General design question: does having a separate inverse dict have some advantages over putting the inverse mapping into the same dictionary as the forward mapping?

E.g. this could have also been implemented as
dict[key] = value
(internally, also do dict[value] = key)

This just made it simpler for me to inspect the dict through the normal repr, it looks like a normal dict but I can access either the key or the value. Your implementation works similarly well, I imagine.

superscore/widgets/page/diff.py

ZLLentz · 2024-12-04T23:53:15Z

superscore/widgets/views.py

+
+
+# Entries for comparison
+class DiffDispatcher(QtCore.QObject, metaclass=QtSingleton):


It's not obvious to me what the value is in having this be a singleton. Will this be created and re-used in multiple contexts? What's the consequence of these contexts not referring to the same singleton object? Would it have been possible to refer to and reference the same object again?

Yes, this is meant to allow us to signal between widgets without navigating through a parent-child chain.

In this case, the views module holds onto the left/right entries used for comparison, and uses that information to create the context menus for the common table/tree views. By having this dispatcher be a singleton, we can subscribe to that signal from the top level Window and open the diff page when the DiffDispatcher emits. We grab this singleton DiffDispatcher (really primarily for holding a singleton signal) in the Window widget and also each of the common views.

Without this, we'd have to fall back to the open_page_slot pattern where we pass a callable through to every page through to the common views. This has already become onerous, and I've considered making the Main window a singleton so that we can access its members easily from any child page (without walking up a questionably connected parent chain).

Would it have been possible to refer to and reference the same object again?

This is what I was trying to achieve with the singleton construction. Do you mean achieving this not with some forced singleton, but with a single object that we make/get with some factory function helper? I thought that this approach was a bit more bulletproof, guarding against unintentional module-level access. (Or maybe I've misinterpreted this comment)

I understand now! Thanks

tangkong · 2024-12-10T22:36:01Z

@ZLLentz Any other thoughts here? Bugging you because Devan is out this week and I want to build on top of this

ZLLentz · 2024-12-11T01:02:28Z

I have nothing else- my questions have all been fully answered and the diff looks nice

tangkong added 9 commits December 3, 2024 15:04

ENH/WIP: first pass diff page, DiffTreeModel with diff highlighting

2be7c57

REF/WIP: refactor DiffRootTree into DiffModelMixin, to be used in oth…

ebb050e

…er diff model subclasses

MNT: use a bi-directional map, fix up data/client setting logic and R…

cfe71a5

…ootTreeView model class specification

ENH: implement table highlighting, alongside some perhaps overcooked …

07085c3

…inheritance and mixins

MNT: touch up highlighting logic, fix bug in BiDict

4ddfb62

ENH/MNT: add QtSingleton for singletons with qtsignals, add ability t…

f08a63b

…o select entries for diff comparison from shared views

TST: add a smoke test for Diff page, I am lazy

3a89d72

MNT: allow Entry as argument to BaseTableDataView.set_data, as is per…

e7aba84

…mitted in models

DOC: pre-release notes, docstring pass

33c27df

tangkong requested review from shilorigins and ZLLentz December 4, 2024 18:42

ZLLentz reviewed Dec 4, 2024

View reviewed changes

BUG: fix BiDict __setitem__ to use real dict methods

8351bf1

tangkong requested a review from ZLLentz December 6, 2024 22:22

ZLLentz approved these changes Dec 11, 2024

View reviewed changes

tangkong merged commit a43a87e into pcdshub:master Dec 11, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GUI: add Diff-view page #112

GUI: add Diff-view page #112

tangkong commented Dec 4, 2024 •

edited

Loading

tangkong commented Dec 4, 2024

ZLLentz left a comment

ZLLentz Dec 4, 2024

tangkong Dec 5, 2024

ZLLentz Dec 4, 2024

tangkong Dec 5, 2024

ZLLentz Dec 4, 2024

tangkong Dec 5, 2024

ZLLentz Dec 4, 2024 •

edited

Loading

tangkong Dec 5, 2024

ZLLentz Dec 5, 2024

ZLLentz Dec 4, 2024

tangkong Dec 5, 2024

ZLLentz Dec 4, 2024

tangkong Dec 5, 2024 •

edited

Loading

ZLLentz Dec 5, 2024

tangkong commented Dec 10, 2024

ZLLentz commented Dec 11, 2024



		# Entries for comparison
		class DiffDispatcher(QtCore.QObject, metaclass=QtSingleton):

GUI: add Diff-view page #112

GUI: add Diff-view page #112

Conversation

tangkong commented Dec 4, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

tangkong commented Dec 4, 2024

ZLLentz left a 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

ZLLentz Dec 4, 2024 • 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

tangkong Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangkong commented Dec 10, 2024

ZLLentz commented Dec 11, 2024

tangkong commented Dec 4, 2024 •

edited

Loading

ZLLentz Dec 4, 2024 •

edited

Loading

tangkong Dec 5, 2024 •

edited

Loading