On 10 October 2013 07:01, Dimitar Zhekov <dimitar.zhekov@gmail.com> wrote:

Hi, all,

I want to discuss how our FiF works, why, and possible improvements.

1. The FiF dialog is created programatically. Is there any reason for
that, or simply nobody cared to XML-ize it? I'm not aware of anything
that can be done gtk+ calls, but can't be done by loading a XML and
less gtk+ calls.

Well, not everybody likes Glade or XML, but why only FIF, why not all search dialogs?

Proposition: move the presentation to glade as much as possible, and
fill the content with code, as in any normal program.

2. The all/project/custom combo box. This thing has 2 functions
dedicated to it, and about 50 LOC in total. So what it does?

all - this is exactly the same as leaving the Files empty.

Yes would seem to be an alias of the delete button on the entry. Are you sure that the delete button is always guaranteed to be present, and not deprecated by GTK3 or removed by themes or ... something else that removes it?

custom - this is exactly the same as entering something in Files.

Well, its "Not Project", ie you need some way to switch away from using project settings, how good "Custom" is as a name is open.

project - this is not exactly the same as placing the Project 'File
patterns' in Files. It pastes them, all right, but then locks the
field, so you can't add / remove / edit a pattern. And the only reason
to lock them is "consistency": the 'project' choice must correspond to
these patterns exactly. Well, if you do a Find, and then switch to
'custom', and then open the Files history, you will be able to edit
the patterns after all... Last and least, 'project' grays the patterns,
making them less visible.

Make sure this functionality isn't important to the project plugin(s). I remember vaguely some significant discussion on the subject during one of the project plugin developments.

The fact that you can edit the paths when you have switched to custom is irrelevant, that doesn't save to the project.

Proposition:
Remove the combo box.
Add a paste icon on the right of files, above the Directory selection
icon, and set it's tooltip to "Paste the project patterns, if any".

This is not the same functionality, choosing project on the combo is a sticky choice that follows the project settings, with this change the user would have to remember to paste when changing the project setting or after a custom search.

FiF checks if a project is open, but not whether it really contains
'File patterns' - RFC.

Which is the project's problem, not the search's. Search should not impose requirements on projects.

3. For non-recursive searches, grep does not allow a directory to be
specified instead of file. To work around that, we read the directory,
back-parse all --include=*.x patterns, and match them manually.

Proposition: grep -rl --include=*.c --exclude-dir=[^.]*
--exclude-dir=.?* void . :)

We pass our Directory as a workind directory to spawn, and the
recursive search already uses . as a grep FILE argument.
--exclude-dir is supported since more than 5 years.

But not in POSIX grep. The hard coded grep options thing has always been a problem (eg see the Note in the manual). The grep setting in tools should accept a whole command like most of the other settings, probably with the usual %something substitutions.

4. Pressing TAB in Files will take you directly to Directory, skipping
the Files history arrow. Very nice, but a TAB in 'Search for' or
Directory will select their history arrows, and Shift-TAB in Directory
will select the Files arrow. Likewise, in the Replace dialog, TAB in
'Search for' will take you to 'Replace with', but another TAB will not
skip to 'Use regular expressions'.

Proposition: TAB in FiF 'Search for' should skip to Directory, and TAB
in Replace dialog 'Replace with' should skip to 'Use regular
expressions'.

Or we should remove these hacks. Personally I find them useful. RFC.

Don't use, don't care, but we probably should be consistent.

5. Pressing ENTER in Directory does not activate Find.

Proposition: obvious.

I thought that was a PR, but I can't find it, nvm.

6. search_find_in_files() checks if the grep command specified in Geany
preferences exists in path.

Proposition: skip that and use G_SPAWN_SEARCH_PATH instead. If we want
the explanation text "Cannot execute grep tool '%s' ...", show it after
spawn fails - that way, it'll at least be correct.

Agree.

Or, if we decide to leave the pre-check, at least change the text to
"Cannot _find_ grep tool '%s' ..."

7. [ ] Extra options. Unchecking this is the same as clearing the text
field, except that you can re-enable them later.

Proposition:
Make 'Extra options' a label.
If we want to keep the options, add a normal history. Personally I
don't care about them.

In the absence of grep in tools prefs being a full command, this is where your --include and --exclude should be the default value. Personally I'd prefer the tools to be a full command.

That's all.

"All" :-)

Cheers

Lex

--
E-gards: Jimmy
_______________________________________________
Devel mailing list
Devel@lists.geany.org
https://lists.geany.org/cgi-bin/mailman/listinfo/devel