Looks nice. I'll have a look. :-)

Hi,

I just checked the branch out, rebased it from HEAD, and I will review all commits, and probably rebase a little your commits (keeping your authorship, of course). The first test looks promising. :-)

It may not take long before integration to master. ;-)

Hi,

That's super. Some of the older commits can probably be squashed. I was new to git then and didn't know what I was doing.

I remember two concrete things that mitch said:
----------
(1). The preferences have to be extracted to the global gimp preferences. Currently Tito has it's own preferences window and preferences file.

(2). There are some globals that need to be cleaned up. Perhaps abstract them into structs and pass them around.

I don't have much context on (1), but I could take a stab at (2) over a weekend. I'm awaiting the review. I wouldn't be surprised if parts of it have to be rewritten :)

Hi,

thanks for the history of Mitch sayings. I was going to ask you these! :-)
Please do not push right now anything. I will want to clean out your work first, then I can push it somewhere, and you can work from the clean branch. :-)

Maybe I can make a fork on github, or something, and I will give you the write rights there (because I don't have the power to give you these for the GNOME git, that would likely depend on Mitch).

A few notes also about git usage:

1/ on a master branch, we want as much as possible not have broken commits. I mean, like commits which would break compilation. A user should be able to checkout at any random commit, and it should compile.
It does not mean that mistakes won't happen. We regularly have a broken master. ;-) But let's not do it *on purpose* at least. :p
I say this because I see your first commit adds references to tito-dialog.[ch] but you only add these files on the next commit. Obviously it breaks. :p

2/ Let's be careful with syntax rules. On my personal projects, I even have different rules. But on a foreign project, I comply to locale rules. :-)

3/ Be careful to trailing whitespaces too. I see a lot of trailing spaces on many of your commits.

If you have color.ui's git config option set to true, you should be able to see trailing whitespaces or tab errors in red in the diff/show output by default. If not already done:
$ git config color.ui true
If you already had the colored output, well check it and fix your tabs and spaces before pushing. :-)

3/ As you say, many commits could be squashed. Though sometimes we may have incomplete features pushed on some special cases, it is really better that the first commit at least would push a working feature (it may still be very basic, but at least not some half-baked feature).
Additional commits may be small if they change different concepts though (actually they should!).

4/ Let's have relevant commit names. They may be short (for instance I am the bad opposite example. I write too much), but let's avoid just "foo". :p
Also since your feature is not alone in master, better to have a "tito" somewhere in your commit descriptions, so that we can easily grep when we search changes to the feature.

Anyway I'll take care of these details just for now, so that you can have a look at the clean version. I suggest not to update too much your own version, to avoid commit conflicts.

By the way, I think we'll want to search for a more neutral name. "tito" is a nice project name, or an internal name, but likely not as a feature name. For people who don't know about it, how can they find the feature by its name? It is kind of ironic that the feature to search things easily with keywords is itself very hard to find with relevant keywords! Probably good keywords are something like "search", or "run", or "menu". Could be "search-run-actions", "search-all-actions", "adaptive-menu"? Or I am not against funny name like "magic-search", or Firefox-like "awesome-search". What do you think? I hope you don't mind if "tito" stays a demo name. :-)

Thanks Jehan. That makes a lot of sense. Some of those commits are very old. Sorry about the 'foo's. :) I've learnt quite some git and good git practices in community projects since then. I'll be sure to keep these things in mind though.

I'll gladly send pull requests to a github fork or maybe if you have a branch on the GNOME git, I could work on that branch and push my code to github. Anything that works for you.

I'm all for a generic name. All the names you proposed are pretty good. How about just "Search"? If tito does expand to search in the manuals, this will be appropriate.

Ahah. Yes simple is good too. :-) Let's go for "search" for now.

Also I may have to massively squash a lot of the commits. I would love having more granularity and have separate commits for specific sub-features of tito, but you have so many commits, and some fix for annoying bugs are so up in the list that it would prove a lot of work to reorder commits, and fix conflicts (because I want each commit to be a solid commit all by itself, this is more important than the feature, in my opinion). I'm not sure spending that much time in cleaning instead of going further is worth it. Let's go for the simple version, which may squash the whole feature as of today in 1 or 2 commits. I hope that's ok with you.

Also do you have an idea why each time I open the Keyboard Shortcuts dialog, I get a popup with a bunch of errors relative to "Gaussian Blur"? I don't have these errors on master, so it must be something related to tito commits, even though I don't see why there would be anything relative to gaussian blur in tito. Is there?

1 or 2 commits works just fine. I do agree that splitting this chunk into granular meaningful commits is not worth the effort.

I'll look into the keyboard shortcuts problem. I remember seeing it too. I'll try fixing it now.

Hi,

In the end, I have squashed all your commits. The feature looks much more readable in a single commit than in several dozen of micro commits. :-)

Also I have spent maybe an hour cleaning with billions of regular expressions (there were so many syntax issues, no way I'd fix them by hand)!

Non exhaustively:
- we try to align function parameters;
- you globally got the indentation logics, but still on many places, the indentation was wrong for some reason;
- we use glib types when possible (for instance gint instead of int, and gfloat instead of float);
- space before parentheses: some_fun ();
- spaces after a comma, before and after =, == or !=, +, etc. Basically try to have a breathing code, which does not look like condensed block pasta without sauce nor olive oil! :-)

And so on (there were many more). You may have a look at the GNU formatting syntax, as well as the file HACKING on GIMP root, which has a few pointer to GIMP coding style.
I'll read in more details the implementation tomorrow. I think there may indeed be stuff to fix. :-)

Hi Jehan,

It's very nice of you to have taken a look at this and fix the syntax. I feel very much responsible for the current state of the code and I'd like to fix this myself wherever possible. I'll read up on the coding style as well. Thank you so much!!

Is there some place I can see the changes you made? How can I also contribute?

Hi srihari!

Sorry for the delay, I do 10 things in the same time!

Also for your feature, I kind of went overboard and updated today with a bunch of ideas (no new features, it does basically the same thing, simply with a design which may be nicer, you'll tell me what you think). I think I can upload tonight 2 patchs. The first one is your squashed commit, which is basically mostly unchanged (except for a lot of cleaning). And the second one is the design update.

Then I propose we wait for Mitch to approve these, and once in the tree, you will be able to work with master as a base. How does this sound?

Sounds great!

Sorry my internet went down yesterday. I'll upload now the patches I talked about.

Name
=====

We said we'd call this "search", but after thought I called it search-action. Just in case we had another kind of search in the future (search images or whatever).

Preferences
===========

With my commit, you'll have the example of how to implement preferences in GIMP.

- Since you added the search feature in the help menu, I added the preferences as a new section in the "Help System" tab, rather creating a new tab. I welcome any feedback on whether that makes sense or not.

- I did not reimplement the "restore defaults" button because there is a general reset feature in the preferences, and I am unsure if a specific reset is worth it. You are welcome to propose this button again if you wish.

- I have slightly more detailed label, because your labels were too fuzzy. You were just naming "width". Was it in pixel? Actually I saw your implementation was in percent. So now I write the unit between parentheses.

- Also your "number of results" was kind of very random. You were just were multiplying by 40, hoping it would correspond. Well it depends on the system, the font used in the UI, etc. :-) Of course you could compute the actual height of a text line in the current UI font, through pango, I guess. But really I did not bother searching how to do so. I modified the implementation to just make it a height in pixel. :p
If you want to change back to a height in "number of results", you are welcome to do it, but with an implementation which actually does what it says (and not only on your specific display ;-).

- I didn't copy the fancy range re-computing on width/position (like, if you increase the width, the position range changes, and such). You are welcome to re-implement it in the right place.

- Since there is no need for a menu anymore on the search window, I simply replaced it with a X (close button). Of course, a Esc/click outside is enough to close it, but since there is nothing now, it does not hurt, and then we are sure the most basic user won't feel lost. :-) Also I made it so it cannot take focus because it was causing issues.

- Finally I added one preferences, which I felt was missing: the number of actions saved in history.

Search Algorithm
=================

- Basically the same, except that now the "show unavailable" preference also applies to history actions (in your current implementation, you would see unavailable actions when they are in your history, whatever the preference says).

- I don't remove the edit-undo* actions anymore from the list. I think they are pretty important. Any reasons why you were filtering them out?
By the way, are there really actions starting with a '<' ?

Design Changes
==============

- More importantly I split the history part and the dialog part.

Now history is dealt entirely in app/widgets/gimpaction-history.[ch]. There is _init() and _exit() functions called at program start/exit. This way we just read the history file (now in action_history) once at startup, and write it once at exit, instead of reading each time the dialog box is called, and writing each time you call an action from it.

- Also history is now closely tied to GimpAction. History is updated as a callback on the "activate" signal of an action. It means we don't log anymore only the actions called through the search dialog, but each time an action is run (for the exception of -menu, -dialog and context- actions).
This way an action the user called through a menu would be also in top position, when he will use the search dialog if he forgot where he called it from.

Bugs
=====

- Not necessarily a bug, but you used too much static variable (you had 13 of them! I could easily drop this number down to 3). I don't say never use them, but sometimes, that's really not worth it.
I'd say you should use them when really you feel not doing so make the whole code much more complex.

- You allocate much too many fixed-sized arrays with huge size. For instance if you want to copy copy a string, don't do this (which was everywhere in your code):

 char *data = g_new (char, 1024);
 strcpy (data, tooltip)

Do this instead:

 gchar *data = g_strdup (tooltip);

It allocates a string for you, with an efficient size. Basically there are a lot of functions in glib to do things much more easily than with the standard C lib (note that even with using the standard lib, doing what you do would not be optimal either!).

- I know this is boring, but when you get a pointer from another function, check its documentation, or better its code, to see if you have to free it yourself. There are many many places in your code where you were not freeing data you should have. That makes as many small memory leaks.

Created attachment 255788 [details] [review]
The cleaned patch of the original author

This is not perfect, with many imperfections still, but this is as much as I could do by using global regexps without reading all. :-)

Created attachment 255789 [details] [review]
The patch above it to change the design and clean further

My additional patch.

You must first commit the first patch, then this one.

Mitch, could you review these?
I think that's a nice feature for GIMP. :-)

Hey Jehan,

I just compiled with your patches against master and tried it out. It looks great! Really clean, readable code. I'm your fan now. :)

Thanks for extracting out the preferences into the global replies. So much neater. I'll keep this pattern in mind if and when I'm working on the preferences. I like what you've done with the history part too. It reads very well.

The algorithm and flow of things in general could be refined over time, but for an initial commit I think this is really solid and hopefully we're going good to ask for Mitch's reviews.

I think we need to have consensus on a shortcut key. How about '/', '?' or Enter?

Also, the UI looks a little raw. I'll see if I can fix this soon.

Yes the algorithm could be improved, but I think it's good enough for a first commit. Once it's in, we can improve it.

For shortcut key, well we could stick with the default (for search) ctrl-f. That comes kind of natural to many users. Now obviously for my usage, I prefer /. This is just one single key (often direct one on many keyboards), it is vim-like, and I see it is currently unused in GIMP. So all good.
Also even though not as common as ctrl-f, we still see it as a quick search in browsers. So some users may still have it associated with "search" in their mind.

I think that both would make sense. In any case, I would likely remap it to / for my own use case, but ctrl-f may still feel more natural to most.
Maybe someone else has an opinion about it?

ctrl+f would be widely accepted. However, wouldn't users be used to ctrl+f for repeating a filter? Are we comfortable with a remapping?

I personally would prefer a single key stroke as well. And '/' fits the bill.

Ctrl-F is also unconfigurable shortcut for searching gradients/fonts/etc in dockable dialogs when they are in list mode.

Created attachment 257637 [details] [review]
Improve Search Dialog

I updated the second patch, which improved the original Tito.

Basically I took into account several of the remarks from everyone.

1/ All the positioning/sizing in the preferences were "too-much", as raised by Mitch. So I decided to go another way.
First I added back the window decorations. That was a point raised by Alexandre Prokoudine too. Though I actually like the simplicity of no-frame, with a title bar, we can easily move and resize the window, which is the second point. I now save the window size and position automatically.
Last point is that to change opacity, the user can ctrl + scroll and this is saved too.
So now the preferences for the search-actions is very minimal, with only the number of actions saved, the checkbox to "show unavailable actions" and the "Clear Action History" button.

2/ As raised by Prokoudine, the search box closing each time you leave its focus may not be appreciated by everybody. I also liked the idea, and this also reminds me of Blender, but I understand some people may dislike. So I removed this behavior. I made so that the search box stays on top though.

The last thing to change would be to make it a generic widget which could be used by other tools. I will do so. But I wanted to update this version first to see if it pleases more people. :-)
Would be nice to have guihipster look at it. :-)

Created attachment 257762 [details] [review]
0002 - Improve Search DIalog

After my previous patch, Téo tried the search feature and got 1 bug (that I fixed now, if I got the right one), and had a few remarks.

Attached my new version of the patch.

The main changes compared to previous patch are:
- Quit button removed: as Téo noticed, with decoration back, it is meaningless.
- Do not keep above other windows anymore: that was kind of annoying when switching windows. I think actually that is up to the user what he does with the search window.
- I added support of a nice idea by Téo: when list focused and text typed, focus text entry back. In effect it means we never lose focus to the text entry, and in the same time, we can use up/down arrows to scroll the list.
- Finally the bug was that text in cell needs to be processed with g_markup_escape_text(). The original author's patch was just checking for existence of '<', but obviously that's not enough.

Created attachment 257888 [details] [review]
0002 - Improve Search Dialog

Some code improvement.

Hey,

I have been thinking it may be easier for guihipster to test from a branch, since he already has a working locale repo. So I took the liberty to create a remote branch origin/tito.

He can just `checkout -b tito` and compile again to be able to get tito, without having to mess around with patches to apply on master.
I hope it is alright?

I've pushed some improvements in the search algorithm. The list is the same, but I've updated the order for better relevancy. Here is the full algorithm:

1/ Any action in the history, which matches the keyword is at the top, ordered by usage (an action used more often than the next one will be above);

2/ Then if the keyword is **exactly** 2 letters, an action whose label's 2 first words starts with these 2 letters is shown first.
This is a shortcut for people who know well the action/effect they search.
For instance "gb" will show first "Gaussian Blur", "cs" would show "Canvas Size" at top, etc.

3/ A label prefixed with the keyword would be shown on top.

4/ Then a label which has the keyword as a substring in label.

For 3/ and 4/, for instance for the search "blur", "Blur/ Sharpen" should be shown before "Gaussian Blur".

5/ Then if the search is a substring in the tooltip.

6/ Finally as a fuzzy search. For instance, let's say I want "Fill with FG Color". I could type "fifg" and that would be my first (and only) result.
This is powerful when you know well what you want, but would return totally irrelevant results otherwise. This is why I made sure this would be shown at the bottom of the list.

I see still some improvements to be made. For instance words could be also re-ordered. For instance currently if you search "blur gaussian", it returns nothing because the current algorithm won't see it as 2 words. I would expect it to return the "Gaussian Blur".
But the current algorithm is already very powerful in my opinion.

Splendid work! It's great to see so much work on tito.
My apologies for being incommunicado for all this while.

Jehan, I remember from an IRC conversation that I needed some write up/screenshots for guihipster. What does this encompass?

Guihipster, have you had a chance to try this out? What are your thoughts?

Hi Srihari,

Well I pushed a public feature branch so that it would be easy for guihipster to test it out instead of just seeing screenshots and description text (since I know he now has a working dev environment).
Hopefully the best would be someone to accompany him in checking this branch out, and testing it (on IRC for instance).

But yeah I would think a few screenshots and description of the feature would be a good accompaniment to real testing too. Now I don't know him very well, so I am unsure what is his favorite working procedure with developers. The best is maybe to ask him directly before. Tell him about the feature, maybe show him the video you already made (even though outdated) for quick idea of what it is about, and tell him it has changed quite a bit, and there is a branch to test in order to get his opinion. Then you see what he tells you.

Hi,

thank you both for your work on this topic. Based on Jehans public feature branch I created a buildjob on our Continuous Integration server (Jenkins). This tells us early, whether the code can be built also on other systems and runs some tests. 
You find it at https://gimptest.flamingtext.com:9090/job/gimp-distcheck-tito/. It is configured to check git every 15 minutes for changes. It would be nice if you checked the build results and fix breaking builds early so we can provide quality software to our testers. Currently there are the same old compatibility tests failing which are fixed in master, so I guess rebasing the tito branch could fix them.
If you need help, feel free to ask in IRC.

Thanks Sven!

I saw you committed to fix the make check. Sorry for this, I always forget to make check in local.

Anyway I still rebased on master too, since the feature branch had been created more than 2 weeks ago. The locale make check worked fine. Let's see on Jenkins. :-)

I've added a specs page for TITo: http://wiki.gimp.org/index.php/Specs:Action_Search_Dialog

I'll send an email to our GUI architect.

I sent an email earlier today to Peter Sikking.

I also improved the search algorithm by normalizing all texts (remove duplicate spaces, remove trailing/leading spaces, etc.), etc. This allows a stronger search which won't be broken because the user hit the space button one time too many (error which may be difficult to detect by the user)

And finally I added word-awareness. Now "blur gaussian" for instance will return "Gaussian Blur". Or course such a result will be considered less relevant than another result with the proper order of words, but at least it will be returned.

I gave it another round of tests today, and it seems to work flawlessly so I guess bugs found earlier are now fixed.

Nice work, really!

I still see some __minor__ interaction glitches:

* After typing a word, the very first "arrow-down key" does nothing useful (only changes the focus from the entry box to the item list). I would expect it to select directly the second item in the list.

* When doing "arrow-right key" while in the item list, the focus go to the "opacity button", which doesn't seem to be something desirable. Instead I would expect the cursor to go one step to the right in the text box, like it is for "arrow-left key"

* The "dialog opacity" _could_ be a good idea in a lot of places, but imho not in that specific feature. I would rather expect that feature in all filters while the user interactively tweaks parameters, and get a live feedback from the preview. But while doing a research with tito, the transparency of the dialog doesn't help. So I suggest we move that (nice) feature from Tito to all filters. But that is anyway something completely unrelated to Tito, and it should properly be discussed before.

Apart from that, it's all really nice and it's something I would love to see in the next major release. I did not read the spec yet, will try to do that in the next days and report back if needed.

Again, thanks Jehan and Srihari!

> I gave it another round of tests today,

Thanks for all the testing!

> and it seems to work flawlessly so I guess bugs found earlier are now fixed.

Cool.

> * After typing a word, the very first "arrow-down key" does nothing useful
(only changes the focus from the entry box to the item list). I would expect it
to select directly the second item in the list.

Hmmm... I see. The first arrow-down is not useful since I made so that the user can always continue to write immediately, though it still does something: it *visually* changes the focus.
But you are probably right that this step could be bypassed.

Actually I'm thinking that maybe the list should never have focus. You could still browse through it with arrows or mouse (and item selection would move), but the focus would just stay forever in the text entry? That could be another idea for a better interface too...

> * When doing "arrow-right key" while in the item list, the focus go to the
"opacity button", which doesn't seem to be something desirable. Instead I would
expect the cursor to go one step to the right in the text box, like it is for
"arrow-left key"

Oh ok, this one is definitely a bug. What you say is supposed to happen (go back to text box) and normally the opacity button was supposed to never get the focus. I just fixed it and pushed.

> * The "dialog opacity" _could_ be a good idea in a lot of places, but imho not
in that specific feature.
 
Yeah I've thought a lot about this feature. Does it really make sense? Is it useless, if not annoying? And if we want it, where should it be?
The fact is that there was this possibility to add window opacity in the original contribution. I did not know if that was really useful, but I liked the idea of a transparent window, which allows to see at all time something more important behind.
But at the time, it was done in its own settings window, and Mitch said, rightly, that any settings should go in the GIMP preference. But then when this feature went in the preferences dialog, we wondered if it was not too much. So I tried to make it back to the main dialog with a nicer UI than a settings dialog.
Anyway maybe the feature altogether was a bad idea and should just be erased. I thought about it a lot too. I just wanted to implement it so that we can see what it does and decide by actually knowing.

Or maybe it could be made into its own specific widget and added in app/widgets for use in other more relevant dialogs (indeed filters, etc.).

Please, the semi-transparent-dialog topic belongs to another discussion. Let's focus on Tito here.

Other minor things I noticed:

* exiting GIMP via Tito warns:
(gimp-2.9:3994): Gtk-CRITICAL **: IA__gtk_widget_destroy: assertion 'GTK_IS_WIDGET (widget)' failed

* Some commands are totally obscure. I guess it's for later, but commands should either be clear (with a small description or not), or not be displayed at all. (eg type in "decrease", and see items displayed).

* Some commands are also available without a suitable context (eg "save and close" or "zoom in" without image, ...), this is probably probably harmless, but still.

I am in a "polishing" mode, and it's probably too early to look so closely.

Also, without typing anything, "arrow down" shows some results? By searching quickly in the spec I did not find the description of that case.

I guess it should show all actions in the history, sorted by "most used"?


Also I read "A value of 100 means that the 100 most used actions will be kept in memory (and not the last 100 run actions)." in the spec. I am not sure if it's right, because if all the 100 slots are "full" with actions used at least 2 times, then no new action can be added unless clearing the history?

> * exiting GIMP via Tito warns:

Fixed now.

> Some commands are totally obscure.

Well I guess that's not the tool's fault. Not all commands have a description indeed. And some command's label are badly chosen. Like "decrease XXX More" => what kind of English is that? Well testing, it appears that "increase/decrease XXX" would "In/Decrease" by 1, whereas "In/Decrease More" by 10. Maybe we should re-label these.

Well that could be a good occasion to review some of the natural language text in our commands (label + description).
For the description, of course we could set tito so that it does not display a command if it has no tooltip. But some commands are still very clear without description. Like "Decrease Color Picker Radius". Well there is a "Radius" spin button in the color picker tool options. This command seems pretty clear to me: it will increase the value there. An additional tooltip with more information is always better, but it does not necessarily mean that the label by itself is completely opaque..

> Some commands are also available without a suitable context

Same as bad labels and descriptions. The culprit is not tito, but the rest of the code. Tito already sees active and inactive actions. The problem with these actions is that they are active while they should not be! This too could be a good easy project: detect all actions which should not be active at some specific context and fix them. :-)

> Also, without typing anything, "arrow down" shows some results? By searching quickly in the spec I did not find the description of that case.

This is the first item in "Keyboard Navigation" in the spec. It shows the full list of actions available, not only history but actions in history are at the top of the list, indeed sorted by most used. I'll add this detail in the spec.

> because if all the 100 slots are "full" with actions used at least 2 times, then no new action can be added unless clearing the history

I thought about it, and that's indeed a problem. I'll have to think about it.

Note: about the actions you noted which should be inactive, I made a patch on bug 719731.

Pushed to master, thanks to all for their efforts!

*** Bug 560716 has been marked as a duplicate of this bug. ***