Helper class for batched / time sliced working on huge text areas

Review Request #128850 - Created Sept. 6, 2016 and updated

Information
Roman Gilg
ktexteditor
master
128592
Reviewers
kate
cullmann, mwolff

This is a follow-up to my review request of improved highlighting in KateSearchBar: https://git.reviewboard.kde.org/r/128592/
The consensus was there, that the highlighting should be done in batches in order to not block the ui.

I did now implement such a behaviour. The helper class BatchedTextWorker from the code review in front of you does the bulk of the work. Since I think such a helper class will be useful in general also for other applications I implemented additional functionality and made it part of the KTextEditor API (tell me if this isn't wanted for some reason).

Features:

  • Works independently of KateSearchBar
  • Define work areas as lines of text or as rectengular blocks (useful for selections in block mode)
  • Specify linear work directions (top to bottom or inverse) and concentric ones (center: middle of input range or cursor position)
  • Control stepsize parameter on-the-fly.

In the example of KateSearchBar it works like this:

  • When asked for highlighting results the bar creates a "BatchedTextWorker worker".
  • The bar connects its highlighting function to worker's ready()-Signal and starts the worker.
  • Along the main event line the worker signals a new portion of work is ready to be worked on by bar.

TODO 16-09-07:

  • Decide about position in code (as part of internal API, directly in "includes/ktexteditor/" or "utils/" ?)

TODO 16-09-06:

  • Work on API documentation
  • Test Stripes mode

I tested the class with KateSearchBar's highlighting functionality, you will find this code shortly after this review goes online in the other one. Not yet tested is the Stripes mode, because it's not needed here. But since in the implementation it's only a small difference to the probably anyway more useful Lines mode, I'll test it further down the line with some other functionality I'm aiming for to implement.

Issues

  • 4
  • 17
  • 3
  • 24
Description From Last Updated
is stipes == block mode? Dominik Haumann Dominik Haumann
The enums are over-engineered: Why do we have use cases that are not used in the first place? If we ... Dominik Haumann Dominik Haumann
Array size of 2? Do we have two beginning cursors? That is: I look at the code and do not ... Dominik Haumann Dominik Haumann
A lambda just to save an if? This does not increase readability, sorry... Rule of thumb: The easiest way is ... Dominik Haumann Dominik Haumann
Anthony Fieroni
Christoph Cullmann
Roman Gilg
Milian Wolff
Roman Gilg
Review request changed

Change Summary:

The worker now works alternating up and down, what shrinks the code. Also other issues from Milian's review fixed.

How it operates now:
1. start() is called client and we find a suitable starting point for our batch working thanks to start(). it then calls processBatch(bool)
2. processBatch(bool) tries to find a new Range (first in the direction not used in the last iteration) by calling backwardLimit() or forwardLimit()
3. In these two fcts the outer Cursor limit of the next Range m_range will be calculated (but if we already hit the input range border and there is no more text in this direction it doesn't set limit and false is returned)
4. If false was returned in step 3 processBatch tries the other direction
5. If both direction returned false, there is no more text to work on in the input range and we we stop the batching
6. Otherwise processBatch() informs the client about the next available Range and invoke processBatch(bool) again, this time first trying out the other direction.

Description:

   

This is a follow-up to my review request of improved highlighting in KateSearchBar: https://git.reviewboard.kde.org/r/128592/

    The consensus was there, that the highlighting should be done in batches in order to not block the ui.

   
   

I did now implement such a behaviour. The helper class BatchedTextWorker from the code review in front of you does the bulk of the work. Since I think such a helper class will be useful in general also for other applications I implemented additional functionality and made it part of the KTextEditor API (tell me if this isn't wanted for some reason).

   
   

Features:

   
   
  • Works independently of KateSearchBar
   
  • Define work areas as lines of text or as rectengular blocks (useful for selections in block mode)
   
  • Specify linear work directions (top to bottom or inverse) and concentric ones (center: middle of input range or cursor position)
   
  • Control stepsize parameter on-the-fly.
   
   

In the example of KateSearchBar it works like this:

   
~  
  • When asked for highlighting results the bar creates an instance worker of BatchedTextWorker
~  
  • The bar connects its highlighting function to worker's ready()-Signal and starts the worker
~  
  • Along the the worker signalling a new portion of work is ready to be worked on by bar
  ~
  • When asked for highlighting results the bar creates a "BatchedTextWorker worker".
  ~
  • The bar connects its highlighting function to worker's ready()-Signal and starts the worker.
  ~
  • Along the main event line the worker signals a new portion of work is ready to be worked on by bar.
   
   

TODO 16-09-07:

   
~  
  • Decide about position in code (as part of API, internal API, directly in folder "src/search" ?)
  ~
  • Decide about position in code (as part of internal API, directly in "includes/ktexteditor/" or "utils/" ?)
   
   

TODO 16-09-06:

   
   
  • Work on API documentation
   
  • Test Stripes mode

Diff:

Revision 3 (+322 -2)

Show changes

Dominik Haumann

Apart from my more detailed comments, a major comment here:

I think the problem of having batch runs for the search is not well enough understood.

Say you search in batches of 2 lines. What, if the user searches for "\n\n\n", or some regular expression that spans an arbitrary amount of lines?

This means: The batch runs need to know the exact range, and kind of a soft range that acts as limit, but this soft limit also allows to expand if necessary.

This logic is not reflected at all by this batch system, and therefore will introduces regressions as is.

I do not yet have a proper solution for this, but as it is right now, I think we need to reject the patch for this very [not so?] obvious reason.

Let's keep the interface internal to KTextEditor for now. Since once it is public, we cannot change it anymore.

If it turns out this interface is needed by other apps, then we can still export it.

Just put the class into the utils folder for now :)

is stipes == block mode?

The enums are over-engineered: Why do we have use cases that are not used in the first place?

If we see we need further tweaking, we can still adapt:

Rule of thumb: If you have code that is not executed, throw it away. Later, if you see you need it, the add it.

PS: Rule that is not really used is also likely to break in case it gets used in some point in time. Then, it is highly likely that someone else is using your code, who is not familiar with it.

Please keep it always as simple as possible...

Pass Range by const '&' reference

Array size of 2? Do we have two beginning cursors?

That is: I look at the code and do not immediately understand it. Please take the liberty and use two KTextEditor::Cursors, instead of packing multiple into an array.

Why? Because currently it is unreadable, and in the .cpp file, it is unclear how big the array is, i.e., what are valid indices? Code that does not use array here is always better.

src/utils/batchedtextworker.cpp (Diff revision 3)
 
 

A lambda just to save an if?

This does not increase readability, sorry...

Rule of thumb: The easiest way is typically the one that is most efficient, and most easy to maintain afterwards.

Here: A lambda that is chosen by a bool index is not good code, please never do that.

Why? Because already the fact that you access an array can potentially lead to an index access that is out of range. Code that does not use an array access here is inherently better.

Loading...