Opened 8 years ago

Closed 7 years ago

#2733 closed enhancement (fixed)

Option to filter message log output by search term

Reported by: chiiph Owned by: sebb
Priority: Low Milestone:
Component: Archived/Vidalia Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Migrated from old trac:
"An option to include a search query as an additional filter would be useful, with standard search operatives eg AND OR NOT etc. "

Child Tickets

Attachments (2)

filter-message-log-search-term.patch (21.3 KB) - added by sebb 7 years ago.
patch to add this functionallity
style-and-other-corrections.patch (5.7 KB) - added by sebb 7 years ago.
coding style and other corrections

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by sebb

Owner: changed from chiiph to sebb
Status: newassigned

Changed 7 years ago by sebb

patch to add this functionallity

comment:2 Changed 7 years ago by sebb

Status: assignedneeds_review

For search term you can use AND, OR, NOT keywords and brackets for grouping.
Regular expressions in perl syntax are optional (its described in line edit tooltip).

comment:4 Changed 7 years ago by chiiph

I haven't finished reviewing this, but the first thing that came to mind was: why not just filter with QRegExp? You'd save all the ExpTree and parsing code. But may be I'm not seeing some advantage this approach has.

comment:5 Changed 7 years ago by sebb

that was the first thing that I thought as well, but then I've started to prepare the regular expressions, and there was a problem - if you can think of reg exp that will capture everything except given set of words, I can change the implementation.
Give me a simple, maintainable regular expression for this one:
NOT x AND NOT y AND NOT z
the one I developed for NOT(x) looked something like:
(?:(?!x).)*$
(maybe slightly different, because I'm making this up right now, I lost the implementation file)
Combine this with AND and OR operators and you will get a mess.
I think parsing to a binary tree is much more readable (and easier to modify) than some complicated regular expressions.

comment:6 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision

A couple of comments:

  • When deleting _expressionTree, you should check that it was created:
if(_expressionTree)
  delete _expressionTree;

The same applies to _logFilter, _left, _right.

  • There is no need to do #include "TorControl.h" if you are not using it.
  • LogFilterSearchTerm has public members, they should be private.
  • This seems an unnecessary variable creation:
+      ExpTree * left = tree.pop();
+      ExpTree * right = tree.pop();
+      ExpTree * node = new ExpTree(s);
+      node->_left = left;
+      node->_right = right;

  • Coding style things:
  • You should leave a space on the left of {
  • In method's implementation, the return type should be on a different line than the signature. Check the HACKING doc for more info on that.
  • If the if statement has only one line, remove the {}, unless they are there for consistency with the rest of the if/then/else.

Changed 7 years ago by sebb

coding style and other corrections

comment:7 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

comment:8 Changed 7 years ago by chiiph

Status: needs_reviewneeds_revision
  • You've missed a couple of {, look for "_logFilter{".
  • The "type in separate line" does not apply to inline defined methods (i.e. bool ExpTree::eval doesn't apply to that rule), sorry for not being clear enough.
  • You should squash the different commits into one (or several, as you see fit). Having commits like this: "Implement X", "Fix X according to Y", "Fix again X according to Z", can be modified to be either: "Implement X" (with all the fixes included), or "Implement X.1", "Add functionality for X.2". Check git rebase -i.
  • You need to add a changes file. See the changes directory, try to follow that format.

If you just give me a branch name in your repo, you don't need to post format-patches, I will just merge your branch in the end.

Great work! We are almost there :)

comment:9 Changed 7 years ago by sebb

Status: needs_revisionneeds_review

Thanks !
I've squashed commits like you suggested. I'm still quite new to git, thanks for the help.
Branch name is '2733_search_filter'
link to final commit: https://github.com/sebthestampede/vidalia/commit/046c9dedd732a4828c31de913d73380aee9ee21f

comment:10 Changed 7 years ago by chiiph

Resolution: fixed
Status: needs_reviewclosed

Merged, thanks!

Note: See TracTickets for help on using tickets.