ENH: find and replace functionality #198

tangkong · 2023-08-21T18:36:23Z

Description

Implements atef-specific find and replace functionality
adds a basic find-and-replace widget
finish up / clean up widget button functionality

Motivation and Context

This stems from a request for templated checkouts, which boils down to having find-and-replace functionality. A naive find-and-replace on the json could touch file/field names, and is rather difficult to inspect.

My intention is to make the future "fill template" step / tool wrap this functionality, possibly with some extra visualization or streamlining buttons. A sample checkout could be run through this tool and and "filled" as is with this tool, I imagine

(In reality I implemented the simple find-and-replace helpers, tried making it more robust, and a week later ended up here.)

Creating a step that fills templates mid-checkout is a possible next step, though we'd have to consider expanding each template step on preparation and inserting it into the tree. Inspection of the substeps of that filled template would be un-inspectable unless we expand in edit-mode, and in that case we're basically creating a bunch of filled template checkouts separately.

How Has This Been Tested?

Some unit tests have been added, more need to be...

Where Has This Been Documented?

This PR

Screenshots

Pre-merge checklist

Code works interactively
Code contains descriptive docstrings, including context and API
New/changed functions and methods are covered in the test suite where possible
Code has been checked for threading issues (no blocking tasks in GUI thread)
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

…nctions

…enums, fine tune row widgets

…aggressively

…fore and after in preview, improve replace behavior

…on init

tangkong · 2023-08-29T23:12:24Z

This is basically feature complete as far as I care for. There was a good amount of gui-ing here so I'm hoping for some adversarial clicking while I write comments / unit tests.

ZLLentz · 2023-08-30T23:01:45Z

atef/ui/fill_template_page.ui

Clickly clicky seemed to work as I used this and it seems very powerful in a way that I think will be well-appreciated by the users. A few thoughts:

Subbing into a big structure can sometimes show a bit too much info. Is there any way we can truncate this so we only show the bit that changed?

Is there any simple way we can automatically refresh when I add a new edit? I got confused for a moment at first before realizing that I needed to click the refresh button on the left.

Oh, also: after saving the file, it seems like this whole template window goes blank. I think that will scare some people. I wonder if we should automatically close this and open the file that was just saved?

Subbing into a big structure can sometimes show a bit too much info. Is there any way we can truncate this so we only show the bit that changed?

I gave this my best effort, but I wonder if I can do any better. Currently it shows the deepest dataclass, which I thought was needed to give the change any context at all. But I suppose now that I have {object}.{attr} on the left I could just show the {attr} on the right

Is there any simple way we can automatically refresh when I add a new edit? I got confused for a moment at first before realizing that I needed to click the refresh button on the left.

This is a perpetual issue I have. I always try to hook more editingFinished signals, but find them to either not catch all the cases I'd want or trigger too often. I'll see what I can do here

I've made the previews show just the bit that changed (the last part of the path), as well as gave the user some notice that their file was saved and that they can open it.

I'll give a bit more information in the details list if the user hasn't picked an edit as well

What would be really neat is if you could preview the checkout bits that changed, maybe even just a color-coded tree of sorts?

Might be neat if the tooltip could show an ASCII tree for the key:

Something to the effect of:

configs[0] - by_attr...

(Weighing an ascii tooltip tree up against the rest of atef functionality... totally top priority, yeah?)

I did think about this, but punted it never to look back. I think it's honestly the ideal visualization, and might not be too much more work, the more I think about it...

This is definitely worth a follow up issue though

ZLLentz

I'm very positive on this. Pending Ken's review I think this is good enough to merge.

ZLLentz · 2023-08-31T00:18:45Z

atef/widgets/config/find_replace.py

+        preview_file = copy.deepcopy(self.data.target)
+
+        try:
+            _ = data.apply(target=preview_file)


Is there some special reason to hold on to this reference?

Ah I dropped my train of thought here. FindReplaceAction.apply catches errors now, so I need that value to decide whether or not application was successful

ZLLentz · 2023-08-31T00:24:03Z

atef/widgets/config/find_replace.py

+        refresh_button = self.button_box.button(QtWidgets.QDialogButtonBox.Retry)
+        refresh_button.clicked.connect(self.refresh_paths)
+        refresh_button.setText('')
+        refresh_button.setToolTip('refresh edit details')
+        refresh_button.setIcon(qta.icon('ei.refresh'))
+
+        apply_button = self.button_box.button(QtWidgets.QDialogButtonBox.Apply)
+        apply_button.clicked.connect(self.apply_edits)
+        apply_button.setText('')
+        apply_button.setToolTip('apply all changes')
+        apply_button.setIcon(qta.icon('ei.check'))


The lack of self.<thing> = <thing> here confused me, but then I realized that the button_box must be tracking them.

ZLLentz · 2023-08-31T00:25:53Z

atef/widgets/config/find_replace.py

+
+            self.actions.append(action)
+
+        # this works, but is smelly and bad.  Access parent table signals


I think the canonical way to do this would be to put a signal on this class, emit it here, and connect it to the parent's parent's signals at some point in the parent's parent, rather than parent twicing. I think this is OK as-is though.

Adding a signal on this row that the table widget expects would force me to change the other instances of this table-row-combo to match (and also have that signal), which I felt wasn't worth it at this juncture. I totally agree though, this isn't quite how things should be done.

ZLLentz · 2023-08-31T00:27:02Z

atef/widgets/config/window.py

@@ -367,13 +374,39 @@ def clear_results(self, *args, **kwargs):

            current_tree.update_statuses()

+    def find_replace(self, *args, **kwargs):
+        """ find and replace in the current tree.  Open a find-replace widget """


I didn't notice this while testing- makes me want to go back and click around again.

this is really nifty

I wonder if anyone will get confused by the interface though- it wasn't obvious to me at first that this wasn't doing a find/replace in the same window, rather it is creating a whole new version that I could choose to edit and save seperately

Yeah, it is really cool but I also felt the same regarding the end user's expectations with the dialog.

"Apply All" seems like it should be replacing it in the file, but it's just doing the changes on an in-memory version until you click 'save as' (... right?)
Maybe the dialog should be something like:

"Choose input file: "
"Choose output file: "
(Make edits in this section)
Reload / Preview and Verify / Save

…er in path)

klauer

The functionality here is awesome! Nicely done. I think this will serve the end user rather well.

Some thoughts, suggestions, nitpicks, and musings as per usual. Nothing at a level that requires holding this PR up any longer. Sorry it took me so long to get around to reviewing!

klauer · 2023-08-31T20:50:38Z

atef/widgets/config/find_replace.py

+    item: Any,
+    match: Callable,
+    parent: List[Tuple[Any, Any]] = []
+) -> Generator:


Suggested change

) -> Generator:

) -> Generator[List[Tuple[Any, Any]], None, None]:

klauer · 2023-08-31T20:50:55Z

atef/widgets/config/find_replace.py

+        self.update_replace_fn()
+
+        self.change_list.clear()
+        self.match_paths = walk_find_match(self.orig_file, self._match_fn)


Careful - you exhaust this below so self.match_paths isn't useful
It'd be better to listify it here, probably, even if just due to the dataclass change comment below on 454

klauer · 2023-08-31T20:57:38Z

atef/ui/fill_template_page.ui

What would be really neat is if you could preview the checkout bits that changed, maybe even just a color-coded tree of sorts?

Might be neat if the tooltip could show an ASCII tree for the key:

Something to the effect of:

configs[0] - by_attr...

(Weighing an ascii tooltip tree up against the rest of atef functionality... totally top priority, yeah?)

klauer · 2023-08-31T21:07:28Z

atef/widgets/config/find_replace.py

+            return
+        elif not edit_row_widget.get_details_rows():
+            l_item = QtWidgets.QListWidgetItem(
+                'provide search and replace text to show details...'


Hmm, it wasn't clear to me why the "edit details" weren't showing:

I think this should probably read "Click on edit in the left list to see details; if nothing shows here there is no match" or something to that effect

klauer · 2023-08-31T21:10:08Z

atef/widgets/config/find_replace.py

+        refresh_button.setToolTip('refresh edit details')
+        refresh_button.setIcon(qta.icon('ei.refresh'))
+
+        apply_button = self.button_box.button(QtWidgets.QDialogButtonBox.Apply)


If I click 'apply all' after having changed a device in the template, I'd have expected to see the device/signal list above change:

klauer · 2023-08-31T21:37:19Z

atef/widgets/config/find_replace.py

+                data = deserialize(ProcedureFile, self._original_json)
+            except ValidationError:
+                logger.error('failed to open file as either active '
+                             'or passive checkout')


This should probably raise and clear original_json
At the very least data is unbound in this except clause

klauer · 2023-08-31T21:38:50Z

atef/widgets/config/find_replace.py

+        self,
+        *args,
+        filepath: Optional[str] = None,
+        window: Optional[Any] = None,


Probably can do better than Any here

TIL typing.TYPE_CHECKING was a thing. circular imports be gone!

klauer · 2023-08-31T21:39:40Z

atef/widgets/config/find_replace.py

+    def __init__(
+        self,
+        *args,
+        data=None,


data unused?

My TableWidgetWithAddRow expected its row widgets to be DataWidgets, all of which have a data kwarg. I probably could have made a dataclass to represent an "Edit" but apparently past me didn't feel like it 🤷

klauer · 2023-08-31T21:41:24Z

atef/widgets/config/find_replace.py

+        """
+        Update the title.  Will be the name and the number of unapplied edits
+        """
+        file_name = os.path.basename(self.fp)


Dealing with None is annoying with respect to the static analyzer but I find it to be good practice - even if you're 100% sure this doesn't get called when fp is None - to guard for it anyway. A future refactor you may forget about it and then get confused why basename(None) is getting called

klauer · 2023-08-31T21:43:46Z

atef/widgets/utils.py

+        self.toggle_button = QtWidgets.QToolButton(text=text)
+        self.toggle_button.setCheckable(True)
+        self.toggle_button.setChecked(False)
+        self.toggle_button.setStyleSheet("QToolButton { border: none; }")


Aside: we should probably move to making an atef stylesheet at some point and move these things there (atef.qss, oh yeah!)

I wonder if that'd fix some of the weird osx rendering differences 🤔 A good idea, but I'm not volunteering right now... 👀

…pare successfully

…hold onto failed components

tangkong · 2023-09-05T20:16:16Z

I think I've addressed all the comments that fit as add-ons to this PR, and made issues for the rest. I'd like to merge this and move on, unless there are any objections

klauer · 2023-09-05T20:18:46Z

Merge at will!

tangkong added 20 commits August 15, 2023 15:46

MNT: move ExpandableFrame to top-level atef.widgets.utils

83ed5c2

ENH/WIP: add prototype find-replace widget, prototype find-replace fu…

4c1a750

…nctions

MNT/GUI: refactor match path finding and replacement methods, handle …

51e5790

…enums, fine tune row widgets

MNT: support regex properly, update match and replace functions more …

8bf8e56

…aggressively

GUI/MNT: generate new file for each preview for sanitization, show be…

4ac23b9

…fore and after in preview, improve replace behavior

MNT: rely entirely on replace_fn

8efb969

TST: first pass find_replace tests

5320994

MNT: swap to more sensible (item, attr) order

d6bc181

TST: add walk_find_path test

25b10cd

GUI: add open_file and open_converted functionality

da5840d

GUI: adjust sizing, add tooltips for before/after

22ca3fd

DOC/TST: docstring/hint pass, add a few more tests

2d6529a

MNT/GUI: make default match/replace fns, FillTemplatePage skeleton-code

ac295ae

REF: FindReplaceAction dataclass (a refactor? in the 4th quarter?!)

618cdae

ENH/MNT: build out FillTemplatePage

f310678

GUI: touch up row widgets, icons

c33333c

GUI: populate device/signal list, add splitters

4793e2f

MNT/GUI: properly clear edits table on apply all, space device_table …

73487f4

…on init

MNT: refresh details on widget interaction

299f033

BUG: actually open a ProcedureFile for device_table...

e10eed5

tangkong marked this pull request as ready for review August 29, 2023 23:11

MNT: add verify button to FillTemplatePage, polish and tweaks

2c82074

tangkong requested review from ZLLentz and klauer August 30, 2023 15:51

tangkong added 2 commits August 30, 2023 08:57

DOC: pre-release notes

354070b

DOC/ENH: type hint and some docstrings, add a forgotten save menu

3addf2d

tangkong changed the title ~~WIP/ENH: find and replace functionality~~ ENH: find and replace functionality Aug 30, 2023

ZLLentz reviewed Aug 30, 2023

View reviewed changes

ZLLentz mentioned this pull request Aug 30, 2023

Enum values are not checked fully correctly #200

Closed

ZLLentz approved these changes Aug 31, 2023

View reviewed changes

tangkong added 2 commits August 31, 2023 12:35

MNT: allow opening from FillTemplatePage, more precise previews (deep…

38d0b59

…er in path)

GUI/DOC: touch up help text and spacing

75e03fc

klauer approved these changes Aug 31, 2023

View reviewed changes

This was referenced Aug 31, 2023

ENH: Allow users to replace found devices directly #203

Open

ENH: make details of FindReplaceRow a tree preview? #204

Open

MNT: refactor find-replace path datatype #205

Open

tangkong added 5 commits September 1, 2023 13:46

MNT: addressing various comments from PR

63ba303

GUI: be more clear about input/output files in FindReplaceWidget

ba7e8f3

MNT: check prepared files for failures properly, they will always pre…

2a8359a

…pare successfully

BUG: SetValueStep should return FailedStep if preparation fails, not …

e1cef2b

…hold onto failed components

TST: allow preparation failures in test_edit_run_toggle

dbdd487

tangkong mentioned this pull request Sep 5, 2023

ENH: Template checkouts #189

Open

tangkong merged commit 91c4076 into pcdshub:master Sep 5, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: find and replace functionality #198

ENH: find and replace functionality #198

tangkong commented Aug 21, 2023 •

edited

Loading

tangkong commented Aug 29, 2023

ZLLentz Aug 30, 2023

ZLLentz Aug 30, 2023

tangkong Aug 30, 2023

tangkong Aug 31, 2023

klauer Aug 31, 2023

tangkong Aug 31, 2023 •

edited

Loading

ZLLentz left a comment

ZLLentz Aug 31, 2023

tangkong Aug 31, 2023

ZLLentz Aug 31, 2023

ZLLentz Aug 31, 2023

tangkong Aug 31, 2023 •

edited

Loading

ZLLentz Aug 31, 2023

ZLLentz Aug 31, 2023

klauer Aug 31, 2023

klauer left a comment

klauer Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

tangkong Aug 31, 2023

klauer Aug 31, 2023

tangkong Aug 31, 2023

klauer Aug 31, 2023

klauer Aug 31, 2023

tangkong Aug 31, 2023

tangkong commented Sep 5, 2023

klauer commented Sep 5, 2023


		self.actions.append(action)

		# this works, but is smelly and bad. Access parent table signals

	) -> Generator:
	) -> Generator[List[Tuple[Any, Any]], None, None]:

ENH: find and replace functionality #198

ENH: find and replace functionality #198

Conversation

tangkong commented Aug 21, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots

Pre-merge checklist

tangkong commented Aug 29, 2023

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 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

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

tangkong Aug 31, 2023 • 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

klauer 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

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

tangkong commented Sep 5, 2023

klauer commented Sep 5, 2023

tangkong commented Aug 21, 2023 •

edited

Loading

tangkong Aug 31, 2023 •

edited

Loading

tangkong Aug 31, 2023 •

edited

Loading