Rewrite of KateSearchBar with focus on object orientation

Review Request #128894 - Created Sept. 12, 2016 and updated

Information
Roman Gilg
ktexteditor
master
Reviewers
kate

Structural overview of proposed new structure

http://i.imgur.com/FcWribI.jpg

Summary

The previous structure of KateSearchBar wasn't really object orientated. The two modes "incremental search" and "power search" were separated more in a C style by different functions and lots of control structures.

Instead we now use KateSearchBar as an interface and container for the two modes. On initialization or change of the mode KateSearchBar creates a new instance of one of the subclasses IncrementalSearchBar or PowerSearchBar of the superclass SearchBar.

In short:

KateSearchBar: The old class now is an interface and container for SearchBar. Holds lasting config data.

namespace KateSearchBarPrivate {

SearchBar: Entry point for KateSearchBar and holds common data and methods.

IncrementalSearchBar: Holds incr search bar ui and methods.

PowerSearchBar: Holds power search bar ui and methods.

}

Major accomplishments with this rewrite:

  • Use of inheritance reduces complexity (I measured around 15-20% less control structures overall)
  • Separate execution paths for finding and replacing content
  • Clean distinction between incremental and power search mode code should easen general maintenance and bug fixing in the future

Fixed bugs relative to current master branch:

  • Cursor jumps to end of word when reentering incremental mode
  • Quick search uses old search term (without visual indication)
  • Switching from incremental to power, changing the pattern here and switching back again to incremental puts cursor at previous incremental search word's end

Saner behaviour on user interaction:

  • Don't set search pattern, when nothing is selected
  • Change search pattern only on reentering mode or in quick search
  • In quick search (F3, no visible search bar), always use selection
  • Jump to new occurence in actual search but not when entering mode
  • Focus on search bar when entering mdoe, but not when searching
  • Select word in doc at current/next position when entering/finding

Questions:

  • Is the idea for the new structure right at all (see diagram at start)? Or is there maybe something fundamentally wrong with it? I'm asking because I'm still in the process of learning to write well structured C++ code.
  • Is using the separate namespace KateSearchBarPrivate for the internal classes the right way to go or should I just use the global namespace and name the internal classes accordingly KateSearchBarPrivate and so on?

Code Repo: Branch "objectifySearchBar" on https://quickgit.kde.org/?p=clones%2Fktexteditor%2Fromangilg%2Fktexteditor.git

Of course I had to rewrite the tests for the new structure. I made comments were I had to change specific values and regarding one specific test, where I had no idea of its meaning.

But most of the time I only had to change the names of variables referencing elements inside KateSearchBar/SearchBar.

* Start testing of SearchBarTest *
Config: Using QtTest library 5.7.0, Qt 5.7.0 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.3.1 20160413)
...
...
...
Totals: 46 passed, 0 failed, 0 skipped, 0 blacklisted, 744ms
* Finished testing of SearchBarTest *

Issues

  • 1
  • 1
  • 0
  • 2
Description From Last Updated
Don't make functions only for autotest. Anthony Fieroni Anthony Fieroni
Roman Gilg
Roman Gilg
Roman Gilg
Review request changed

Change Summary:

Added some questions, which can be answered without a full code review. Corrected link to repo.

Description:

~  

Structural overview of new structure

  ~

Structural overview of proposed new structure

   
   

http://i.imgur.com/FcWribI.jpg

   
   

Summary

   
~  

The previous structure of KateSearchBar wasn't really object orientated. The two modes "incremental search" and "power search"

  ~

The previous structure of KateSearchBar wasn't really object orientated. The two modes "incremental search" and "power search" were separated more in a C style by different functions and lots of control structures.

-   were separated more in a C style by different functions and lots of control structures.

   
   

Instead we now use KateSearchBar as an interface and container for the two modes. On initialization or change of the mode KateSearchBar creates a new instance of one of the subclasses IncrementalSearchBar or PowerSearchBar of the superclass SearchBar.

   
   

In short:

   
   

KateSearchBar: The old class now is an interface and container for SearchBar. Holds lasting config data.

   
   

namespace KateSearchBarPrivate {

   
   

SearchBar: Entry point for KateSearchBar and holds common data and methods.

   
   

IncrementalSearchBar: Holds incr search bar ui and methods.

   
   

PowerSearchBar: Holds power search bar ui and methods.

   
   

}

   
   

Major accomplishments with this rewrite:

   
   
  • Use of inheritance reduces complexity (I measured around 15-20% less control structures overall)
   
  • Separate execution paths for finding and replacing content
   
  • Clean distinction between incremental and power search mode code should easen general maintenance and bug fixing in the future
   
   

Fixed bugs relative to current master branch:

   
   
  • Cursor jumps to end of word when reentering incremental mode
   
  • Quick search uses old search term (without visual indication)
   
  • Switching from incremental to power, changing the pattern here and switching back again to incremental puts cursor at previous incremental search word's end
   
   

Saner behaviour on user interaction:

   
   
  • Don't set search pattern, when nothing is selected
   
  • Change search pattern only on reentering mode or in quick search
   
  • In quick search (F3, no visible search bar), always use selection
   
  • Jump to new occurence in actual search but not when entering mode
   
  • Focus on search bar when entering mdoe, but not when searching
   
  • Select word in doc at current/next position when entering/finding
   
~  

Code Repo: Branch "objectifySearchBar" on https://quickgit.kde.org/git@git.kde.org:clones/ktexteditor/romangilg/ktexteditor.git

  ~

Questions:

  +
  +
  • Is the idea for the new structure right at all (see diagram at start)? Or is there maybe something fundamentally wrong with it? I'm asking because I'm still in the process of learning to write well structured C++ code.
  +
  • Is using the separate namespace KateSearchBarPrivate for the internal classes the right way to go or should I just use the global namespace and name the internal classes accordingly KateSearchBarPrivate and so on?
  +
  +

Code Repo: Branch "objectifySearchBar" on https://quickgit.kde.org/?p=clones%2Fktexteditor%2Fromangilg%2Fktexteditor.git

Anthony Fieroni

Block diagram looks good, overall code base doesn't. When you make big changes, like this, try to minimize overall code i.e. try to make it smaller, easy readable and maintainable. So if you can make it more clear, it can be accepted.

  1. Thank you for your review! When you say the overall code base should be made more clear, you mean the code in the header and the cpp file itself, right? Should I try to comment it more or maybe distribute the classes on several files? I don't know if I can shrink the overall loc. I already was keen on minimizing loc as much as possible in the first revision. But if you have some idea (like the unnecessary setWidget() fct), please tell me. :)

  2. Yeah, the code base must be compact, maintainers don't want more complicated code even if he works better. Focus on simplificy it, remove legacy code when you don't needed it or do it in better approach, remove autotest only functions, remove duplicate code.

src/search/katesearchbar.cpp (Diff revision 1)
 
 

Not needed, just call it where is needed.

src/search/katesearchbar.cpp (Diff revision 1)
 
 

Don't make functions only for autotest.

  1. I didn't make these functions. They are in KateSearchBar on master branch, too. And there they are only used for autotests aswell.

Loading...