Add button to re-run search in files
ClosedPublic

Authored by Sergobot on Nov 30 2016, 5:25 PM.

Details

Summary

Add button to re-run search in files, so a user doesn't need to search again through the dialog.

Test Plan

Works fine, just as expected. All the tests were passed.

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
Sergobot updated this revision to Diff 8640.Nov 30 2016, 5:25 PM
Sergobot retitled this revision from to Add button to re-run search in files.
Sergobot updated this object.
Sergobot edited the test plan for this revision. (Show Details)
Sergobot added a reviewer: kfunk.
Sergobot set the repository for this revision to R33 KDevPlatform.
Sergobot added a subscriber: kfunk.
kfunk changed the visibility from "KDevelop (Project)" to "Public (No Login Required)".
kfunk edited edge metadata.Nov 30 2016, 6:37 PM

I think we need a more sophisticated approach. I suggest creating a new struct GrepJobSettings which holds all settings you can specify in a search dialog. Then, remove all the individual setters/getters (e.g. GrepDialog::patternString, GrepJob::setPatternString) in both GrepDialog/GrepJob and instead just use GrepJobSettings in both places.

To store the last used settings one could add another field to GrepPlugin, e.g. m_lastGrepJobSettings;. Makes sense?

plugins/grepview/grepoutputview.cpp
283

This will use the pattern from the current selection instead of the last search pattern. This is not what we want unfortunately.

kfunk requested changes to this revision.Nov 30 2016, 7:00 PM
kfunk edited edge metadata.
This revision now requires changes to proceed.Nov 30 2016, 7:00 PM
Sergobot updated this revision to Diff 8698.Dec 2 2016, 4:07 PM
Sergobot edited the test plan for this revision. (Show Details)
Sergobot edited edge metadata.
Sergobot changed the visibility from "Public (No Login Required)" to "KDevelop (Project)".

Moved most of GrepDialog's and GrepJob's set/getters away, replacing them with a nicer setSettings()

Sergobot edited the test plan for this revision. (Show Details)Dec 2 2016, 4:08 PM
Sergobot edited edge metadata.
Sergobot changed the visibility from "KDevelop (Project)" to "Public (No Login Required)".

Some nitpicks. :)

plugins/grepview/grepdialog.cpp
442

Unneeded indentation change.

plugins/grepview/grepdialog.h
36

Probably it's better to use "const GrepJobSettings& settings" like in GrepJob.

plugins/grepview/grepjob.h
47

Maybe some more readable name? Something like "template" or maybe "searchTemplate".

68

What's point of using comments here? It's not necessary since old version still be available on git in case it'll be needed.

121

Same there. Indentation issue.

123

Is m_templateString used somewhere now? If no, please remove it.

Sergobot updated this revision to Diff 8711.Dec 2 2016, 10:11 PM
Sergobot marked an inline comment as done.

Fixed all the issues by @ematirov and a bug caused by previous revisions of the diff. The bug was all about providing up-to-date data to GrepJob in GrepDialog::startSearch()

Hm... Did you test it?

It doesn't work for me:

  1. Open a project.
  2. Search in files
  3. 2 matches found.
  4. Click "refresh"
  5. no matches and search string is empty.
Sergobot updated this revision to Diff 8716.Dec 3 2016, 1:09 PM
Sergobot marked 5 inline comments as done.

Now it works again :/

Sergobot updated this revision to Diff 8720.EditedDec 3 2016, 2:36 PM

Fix some whitespace issues

LGTM now.
Please, wait for @kfunk's review.

kfunk accepted this revision.Dec 16 2016, 9:31 PM
kfunk edited edge metadata.

Rest LGTM, please push after fixing those minor issues.

Great change, thanks!

plugins/grepview/grepdialog.cpp
452

filesOnly = enabled && checked?

plugins/grepview/grepjob.h
39

Minor style issue: Newline before {

This revision is now accepted and ready to land.Dec 16 2016, 9:31 PM
Sergobot updated this revision to Diff 9096.Dec 16 2016, 10:16 PM
Sergobot marked an inline comment as done.
Sergobot edited edge metadata.

Fix issues by @kfunk

Sergobot marked 2 inline comments as done.Dec 16 2016, 10:20 PM
Sergobot updated this revision to Diff 9098.Dec 16 2016, 10:28 PM

Now it's a hotFIX

Sergobot updated this revision to Diff 9099.Dec 16 2016, 10:30 PM

Sorry for being so silly, now it should be ok

This revision was automatically updated to reflect the committed changes.