sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[office] Use incremental search. #103

Closed dcaliste closed 7 years ago

dcaliste commented 7 years ago

Implement an incremental text search by signaling every 3 pages the additional matches since the last update. A singular modification is that now the search model advertised by the document is available since the beginning of the request with a zero count. A fraction of the job done is also signaled so one can implement a progress bar.

I had to change various things in the UI because some of the logic in SearchBarItem.qml was based on the assumption that the search model was given at the end of the search and that a count of zero was meaning no match. The clear / cancel button is now also a stop button that can stop the current search while keeping the results already found if pressed when searching.

I think, the biggest other change I made is that the search model is not anymore a reference on the search list owned by the thread but a copy that is slowly increased.

dcaliste commented 7 years ago

I've addressed all your suggestions of improvement. In addition, I've also modified:

pvuorela commented 7 years ago

the PdfDocument::search() function to emit a searchModelChanged() signal with a null value before emitting it with the true new object. I've encountered cases while testing when the call to new provides the same memory address than before, making the onModelChanged signal of the display model not emitted.

Sounds strange, sure about that? Should be c++ responsibility checking when signals need to be emitted.

dcaliste commented 7 years ago

Sounds strange, sure about that? Should be c++ responsibility checking when signals need to be emitted.

Sorry, I may be inaccurate in my description. The signal is indeed emitted each time by the C++ side, even on the same address.

The issue is on QML side in the Repeater, where we have model: doc.searchModel. There, the onModelChanged signal is emitted (not the searchModelChanged from the document), only if doc.searchModel comes from a different memory address.

You may test this with the previous version, by searching 'fgjgfg', see that it fails, then reopen the searchfield and clear with back key (not the clear button), and search for another valid string. Maybe add a console.log(doc.searchModel) on searchModelChanged to check that the memory address is the same. It's not happening all the time.

pvuorela commented 7 years ago

The issue is on QML side in the Repeater, where we have model: doc.searchModel. There, the onModelChanged signal is emitted (not the searchModelChanged from the document), only if doc.searchModel comes from a different memory address.

Oh right, so in other words it makes no difference here since onModelChanged is now removed? In that case up to you whether to have intermediate null pointer state. Maybe comment could be slightly adjusted to cover the specific case.

On general, think this is looking good. Can merge whenever you see fit.

dcaliste commented 7 years ago

it makes no difference here since onModelChanged is now removed?

Not exactly, because in my tests, the repeater itself was broken because from my understanding it was not reacting anymore to any count changes.

I guess the C++ implementation look something like this:

repeater = new Repeater;
connect(document, searchModelChanged, repeater, onModelChanged);

onModelChanged(model)
{
   if (model == this.model) return;

   connect(model, countChanged, this, update);
}

We can see with that implementation that when document.model dies, the update connection is removed. Then when document.model is new but with the same memory address, the if line in the slot raises the bug with the repeater not being updated and not reacting anymore to count changes.

I've tried to make the comment a bit more explicit.