Find and replace doesn't work in EEschema

Bug #1531163 reported by Art
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
KiCad
Fix Released
Low
Chris Pavlina

Bug Description

When I attempt to do Find and Replace (for example I want to replace all names of components staring with #TP to TP) the Replace and Replace All buttons always stay greyed out.

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Can confirm. It un-greys if you first click Find, though you shouldn't have to do that.

Changed in kicad:
status: New → Confirmed
importance: Undecided → Low
assignee: nobody → Chris Pavlina (pavlina-chris)
Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Patch attached. Can another dev please test this before one of us commits it? The fix feels a bit too simplistic to me, and I'm unfamiliar with that specific area - but it worked fine in the test cases I could think of.

Changed in kicad:
status: Confirmed → In Progress
Revision history for this message
Wayne Stambaugh (stambaughw) wrote : Re: [Bug 1531163] Re: Find and replace doesn't work in EEschema

This doesn't resolve the user's issue. The current behavior works as
designed. Wildcards are not currently supported. It is a pure text
search. Both the find and find all buttons are enable after the find
action actually finds a match. If no match is found, the find and find
all buttons remain disabled. I designed it this way because I prefer
this method of feedback versus popping up a nag dialog to warn the user
there is nothing matching the search string. We can debate whether this
makes sense or not but the current design is working properly.

There is code already in place (it has not been tested) to use wxRegEx
for searches but I never got around to hooking up this code because
there has never been much interest. One potential problem would be that
this is a full regex implementation rather than wildcards such as * and
? which more users are comfortable with. I've always been leery about
exposing typical users to regular expressions. I don't want to be
responsible for anyone's head exploding.

On 1/9/2016 9:19 PM, Chris Pavlina wrote:
> Patch attached. Can another dev please test this before one of us
> commits it? The fix feels a bit too simplistic to me, and I'm unfamiliar
> with that specific area - but it worked fine in the test cases I could
> think of.
>
> ** Patch added: "replace_all.patch"
> https://bugs.launchpad.net/kicad/+bug/1531163/+attachment/4547183/+files/replace_all.patch
>
> ** Changed in: kicad
> Status: Confirmed => In Progress
>

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

I'm a bit confused by the comments about regex searches, did someone mention that? I certainly didn't enable it in my patch.

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

I think you might have mixed up this bug report with https://bugs.launchpad.net/kicad/+bug/1531479 ?

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

Duh!? I need to start going back and looking at the original bug report
before I reply. However, the first paragraph is still valid for this
bug report. The replace feature is not broken. It just doesn't work
the way the user expected it to. I'm not opposed to always enabling
replace and replace all buttons but adding a nag dialog to warn the user
that nothing was replaced is a shooting offense. I know your patch
doesn't do that but there always seems to be a tendency to add them.
The current design gives you feedback about matches against the search
string. With your patch, nothing would happen if no matches were found
against the search string. The user may not even notice. If you think
this is a better UI design, go ahead and commit it.

On 1/11/2016 3:28 PM, Chris Pavlina wrote:
> I think you might have mixed up this bug report with
> https://bugs.launchpad.net/kicad/+bug/1531479 ?
>

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Honestly, I think it's okay. I see what you're saying about the user not really being aware it didn't replace anything - but that's already implied with a blind find-and-replace, it never tells the user what's been replaced. It's always a silent operation, so it's really already up to the user to check that it did something sensible.

Since you don't mind, I think I'll go ahead and commit - though I don't want to be too pushy about the UI stuff all the time, if you really don't like it that's fine.

Agreed about the nag screen. A change summary is fine sometimes (as I suggested on the related bug), but a special purpose "yo, this didn't do anything" is just silly.

Side note: currently a "replace all" seems to push each individual replacement onto the undo stack. What's your opinion on having a single undo item for the whole operation? That seems to be what most of the programs I use do.

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

(The side note probably would be better broken off into a mailing list thread or something. I'm not sure why I stuck that in a comment. Not the greatest idea.)

Revision history for this message
Art (diametrix) wrote :

@Wayne

It gives me no additional information to be able to find the first match before allowing me to do Replace All. If I want to replace 42 entries all of which start with the same characters, what benefit is it to me to be able to see the first one? The cost is confusion of the user (I've been using KiCad for years and still didn't know how to use Replace function!) and an extra operation. What would be actually beneficial is to see how many entries have been replaced at the end (which would've been done through that popup window that you so easily dismissed) We are not talking about some convoluted algorithms here. Why do you guys always try to reinvent the bicycle? How does your "Replace All" function work in your C++ editor? I bet you don't have to press Find first only to see the first hit and only then to be able to do Replace All only to be left wondering how many actual entries have been replaced.

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

On 1/11/2016 4:11 PM, Chris Pavlina wrote:
> Honestly, I think it's okay. I see what you're saying about the user not
> really being aware it didn't replace anything - but that's already
> implied with a blind find-and-replace, it never tells the user what's
> been replaced. It's always a silent operation, so it's really already up
> to the user to check that it did something sensible.
>
> Since you don't mind, I think I'll go ahead and commit - though I don't
> want to be too pushy about the UI stuff all the time, if you really
> don't like it that's fine.

Go ahead and commit it.

>
> Agreed about the nag screen. A change summary is fine sometimes (as I
> suggested on the related bug), but a special purpose "yo, this didn't do
> anything" is just silly.

If you add a summary, please make it a static text control (or some
other control) in the find/replace dialog rather than a pop up.

>
> Side note: currently a "replace all" seems to push each individual
> replacement onto the undo stack. What's your opinion on having a single
> undo item for the whole operation? That seems to be what most of the
> programs I use do.
>

I was lazy when I wrote this. It will be a bit tricky because each
replace is done incrementally in the . Your best be may be do make them
into a block and perform a block undo. I'm not sure how easy this will
be to pull off. Please do not move forward on this until after I've
completed merging my Eeschema refactor work or there will definitely be
conflict issues.

Revision history for this message
Chris Pavlina (pavlina-chris) wrote :

Yeah, taking advantage of the block operation was my first guess at a way to do it. But yes, I'll definitely hold off on this - don't want to step on your toes with the refactor, and I still have a decently long todo list anyway :)

Changed in kicad:
status: In Progress → Fix Committed
Changed in kicad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.