Add global search

[moisesjbc]

Hi!

I am working on a global search feature by extending the current Outline search to all models.

What I have so far

• Added filtering columns for Character and "Flat data" (sentence, sentence, paragraph, page and full summaries of the book).
• Added full word search option.
• Navigation and highlighting of searches results when clicking on them (*).
  • (*) More on this on the ToDo part.
• Shortcut CTRL+F and search action added to Edit menu.

ToDo (not an exhaustive list)

• Expand search to all models.
• More unit testing.
• Add regex search (Any plans to support regular expression searches? #682 )
• Implement feedback from Adds a simple text find dialog. #681
• Improve filters menu. Right now all filters are displayed in a long menu on search widget. I am thinking on splitting them into submenus "Outline", "Character", "Flat Data", etc. What do you think?
• Improved search result highlighting. Right now, searches on Outline labels, POVs and statuses highlights the label instead of the dropdown because I couldn't make it work for the latter. Any ideas on this?
• Fix paths in search results list (ie. I am thinking on adding [Model type] > ... to the path. And maybe the model column at the end?)

Some questions

• In general, what do you think of the changes so far? Something important that I forgot to mention on ToDo (it is not an exhaustive list anyways)?
• I see that classes and methods are named resembling Qt names (camelCase) but in multiple places I couldn't resist using Python convention ^^U. This is something I have to normalize in my PR. Should I stick to the Qt naming convention?

[moisesjbc]

I don't know why the build is failing on Semaphore ^^U Have anyone seen that error before?

[gedakc]

I don't know why the build is failing on Semaphore ^^U Have anyone seen that error before?

Thanks @moisesjbc for this enhancement. And don't worry about Semaphore. The Semaphore automated tests were configured by the original author and have been failing for quite some time. Unfortunately we do not have the ability to see or change the Semaphore stuff so we must wait until the problem catches the original author's attention. My understanding is that the original author is very busy and no longer has time for the project. Further the macOS X automated builds using Travis CI are also failing so you can safely ignore that component of the tests for now.

Regarding naming conventions my recommendation is to match the existing code base, regardless of one's own personal preferences. Mixing naming standards mostly makes the code harder to read.

I'm working on releases for a few projects so it will be a while before I can look into the backlog of Pull Requests for this project. In the meantime please feel free to work on the code puzzles you've identified and to update this PR as you progress.

Cheers,
Curtis

[moisesjbc]

Hi @gedakc thanks for the info!

I'll keep working on the PR and trying to match the existing code base convention.

[moisesjbc]

Hi @gedakc !

I just wanted to let you know that now I consider the PR ready for reviewing.

Don't hesitate to tell me anything that may be improved.

Bye!

[TheJackiMonster]

Could you squash the commits for the review and merge? This would help. I will look at the current state in mean time.

[TheJackiMonster]

Okay, so I have tested it in my own project and it works great so far. The only problem I see is the general placement of the search:

• If it is placed on the left or right site, scrolling through the results is great but names can be cut off.
• If it is placed on bottom or top, the view is wide enough on most systems to fit all kind of names but scrolling is required to see most results.

I suggest adding an overlay if you hover over the results to show the full name of the matches location and maybe even some context, if found in the texts of a chapter. So users don't have to click on each result to find the location they look for.

Edit: Also when you have Search and for example Navigation grouped/paged as views and you press Ctrl+F or click on the search button in the menu. The Search view won't show up. So this could be implemented as well.

[moisesjbc]

Wow, I didn't expect a response after all this time! :-P

Thanks @TheJackiMonster for checking it and the feedback! I will try to find time during the week and implement the suggestions.

Bye!

[TheJackiMonster]

I have also found this issue #480 which lists a few ideas to change about the search. I'm not sure about highlighting matches because this would require the feature to highlight anything in the text view without colliding with spell checking. It also requires clearing the highlighting closing the search or changing the term without clearing the highlight of the spell checking.

So the first requested part could be more difficult. The second part is about adding hot keys to jump between found matches forwards and backwards. I think this would be quite useful.

[moisesjbc]

Hi @TheJackiMonster

As suggested, I added a tooltip to the search results indicating the result number, the path and some context. It is still a WIP but I would like to know what do you think about it so far.

Bye!

[TheJackiMonster]

Currently I get following Error when trying to search:

Traceback (most recent call last):
File "/home/thejackimonster/github/manuskript/bin/../manuskript/ui/search.py", line 74, in search
results += model.search_occurrences(search_regex, filtered_columns)
File "/home/thejackimonster/github/manuskript/bin/../manuskript/models/searchableModel.py", line 10, in search_occurrences
results += item.search_occurrences(search_regex, columns)
File "/home/thejackimonster/github/manuskript/bin/../manuskript/models/characterModel.py", line 357, in search_occurrences
results.append(self.wrap_search_occurrence(column, i, i))
TypeError: wrap_search_occurrence() missing 1 required positional argument: 'context'
Fatal Python error: Aborted

[TheJackiMonster]

Okay, so the error was caused when I searched for the name of a character and it could not add any context for a result found in the character model. So this has to be fixed.

Otherwise I think it's looking quite good. The tooltip could probably last a little longer depending on the length of its context. Maybe even show it until you leave the matching entry or hover over another.

I think the Qt API is messing something up there when you show the tooltip from the overriden render call. But it works quite good if you use mouse-over events like in ui/views/textEditView.py for the more advanced spell checking.

Also if the context would include where the match was found exactly, that would be good. For example if I have a match in the title of a chapter it tells me currently:

#N - Example Chapter
Example Chapter

..but I think something like this would help:

#N - Example Chapter
Title: Example Chapter

Another addition could be some dots before and after a cutout from the text the match was found in.

#N - Example Chapter
...
Once upon a time it was a
...

..or maybe in a shorter version (less lines for a smaller tooltip is probably better):

#N - Example Chapter
...Once upon a time it was a...

#N - Example Chapter
[...] Once upon a time it was a [...]

I personally would probably prefer the citation-like version because it is quite unlikely someone uses this syntax inside of a text, especially in fiction I assume. But something like this makes it more clear, what's title of the tooltip and what's the context. Making the match formatted bold is pretty neat already.

I would also think for most consistency it should not include some empty lines in the context (better cutting of by splitting at new line characters and putting only the text containing the match as context). After that I think it's ready to get merged. It will definitely be an awesome feature.

[moisesjbc]

Hi @TheJackiMonster, thanks for the feeback!

Just a question: what do you mean by "I think the Qt API is messing something up there when you show the tooltip from the overriden render call"? What are you seeing?

I tried moving the logic to an editorEvent() in the delegate (*) but I see the same behavior for the tooltips here. What am I missing?

Thanks!

(*) mouseMoveEvent() didn't work, I guess that it is because the delegate is not a widget, but I am not sure.

[TheJackiMonster]

I'm not completely sure because it's been a while since I've implemented the tooltip for the spell checking. But I think Qt only allows one tooltip at a time. So when you open up multiple tooltips (even if it is the same one at the same position) it does not just stay open.

At least what I'm experiencing is that I hover over an entry, the tooltip opens and closes after a view seconds. So I assume the tooltip is opened in the paint-event, rendered above and this causes another paint-event which will render over it with a different state, so the tooltip does not flicker but is getting closed. I am not sure though but I'm relatively certain triggering other UI like a tooltip should not happen in something as a render-call. At least from other UI I have used, it is quite common to separate any interaction from actual rendering.

[moisesjbc]

Hi!

You are right about not calling the tooltip in a render call. I moved it to event handlers.

Quick update about last changes:

Done

• Added translated path to results in characters, outline and "flat data": I added to the end the column where the search result was found, so the tooltip is displayed as

    Characters > <character name> > Summary
  
    <context>`
  

• Now the is the line including the match.

• Added the [...] surrounding the context.

• Added key shortcuts for navigating the results (F3: next, Shift+F3: previous)

• Added missing call to _translate for options in the filters menu.

ToDo

• Add missing search fields (ie. all fields from World models)
• Add regex search
• Fix "highlighter" for some cases
• Other minor fixes

[TheJackiMonster]

Looks really good. ^-^

[moisesjbc]

Hi!

Sorry for activating the ghost protocol :-P I rushed today and I think I finished the development :-)

Tomorrow I will try to squash all the commits into one, hoping that I don't make a mess.

Bye!

[moisesjbc]

Hi @TheJackiMonster !

I think that its is ready for review now. What confuses me a bit are the differences in i18n/ directory. I expected all the changes in the line="" attributes, but I see some texts added / removed that I don't know if they are OK because of the rebase or not ^^U

Thanks!

[TheJackiMonster]

I think the changes in the .ts files should not be problematic since they are automatically generated from what I know. I will test the commit then and merge afterwards. ^^