This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106363/

On September 10th, 2012, 6:58 p.m., Michel Ludwig wrote:

Thanks for changes! Here are a few comments:

1. Instead of using strings, I'd do everything with enums. For example, have the following methods in LaTeXOutputHandler:
   enum BibliograhpyBackendSettings {AUTO_DETECT_BACKEND, BIBTEX_BACKEND, BIBER_BACKEND}
   
   BibliograhpyBackendSettings bibliographyBackendTool();
   void setBibliographyBackendTool(BibliograhpyBackendSettings s);

   void setAutoDetectedBibliographyBackendTool(const QString& s);
   QString autoDetectedBibliographyBackendTool() const;

   Also, I think that BibliograhpyBackendSettings should only be a very light class, only there for reading/storing settings. So, the logic for auto-detecting the tool should go back to the LaTeX tool class.
2. The KSelectAction needs to be added to a action collection (see one of the 'createActions' methods). If you give it a name like "set_bibliography_backend" in the addAction method, you can add it in "kileui.rc" (the version of that file also has to be increased in the 'kpartgui' tag).
3. There is need to access menus directly. This can all be done using KXMLGUI techniques (see 2.).
4. I think it should be enough to use to the "General" group. This will also avoid having to take of varying group names.
5. That's because Kile also works with single documents. It's of course possible to add the backend settings to the project configuration dialog.

It seems a bit complicated at the moment to store the backend settings for single documents. I'll think about how this can be done best.

On September 10th, 2012, 9:30 p.m., Eugene Shalygin wrote:

Dear Michel,

thanks for the review, first of all! 
1. It seems like we have a bit different points of view on this backend question: I would absolutely agree with all of your suggestions about enums and locations of the code if I'd be sure that there will be no other backends. Is it really the case? I was not sure and that is why I use strings (and a map). If one would need to add one more backend (bibtex8? bibtexu?) then that part would need adding new identifier only, but not two new functions. Also, in the current logic of backend selection one needs something to say "no data avaliable". Empty string is easy for that. Enum value "NO_BACKEND_INFORMATION" is ugly for me, exception (logically ideal) is too heavy for that. That is another reason why I choose strings.
Regarding item "2" I'm grateful and will try.
3. Do not understand, sorry.
4. As you wish.
5. I remember Visual Studio was creating projects is memory to make use cases identical... That was quite nice

On September 11th, 2012, 7:21 a.m., Michel Ludwig wrote:

1. Enums are perfect for the use case that exactly one of several options has to be selected. For that purpose, strings require more memory and they cannot be used in "switch" statements, which would also ensures that all possible cases of the enum are covered. Furthermore, strings can hold unintended values, which can lead to strange bugs (and this happened within Kile already ;)).
As far as I can see it, the use case of "no data available" is only relevant for the auto-detection of the bibtool, and for that "autoDetectedBibliographyBackendTool()" returns a string. So, it's still possible to return an empty string there.
If you want, you can also use a QSignalMapper. Then there is no need to add new functions (but even for our handful of bibtools that shouldn't be a problem :))
3. This is basically just the same as 2. The technology to do that is called KXMLGUI.
5. Yes, we can think about introducing "default projects" for Kile 3.1 or so, but at the moment, I don't want to touch that. We have to get version 3 out! :)

On September 11th, 2012, 11:45 a.m., Eugene Shalygin wrote:

1. Then what will be stored in a project file as a tool identifiers? Numbers? I think it is not a good idea: they are unreadable by a human.  If we want strings, then  LaTeXOutputInfo should convert enum values into strings and we came to almost the same sequence plus function to convert enum to a a string. And, in ane case, enum value will be converted to a string on each run of LaTeX. So, efficiency will be more or less the same, because we need string on each LaTeX run to update config value. Sure, that would be good to use "switch" instead of sequence of "if"s. Unintended values, surely, must be taken into account. So far the only source for them - manual editing of a project file (because only one function that generates string values returns one of the constants). And this is possible with enums also. But I understand your point. 

Now I think that just a set of string constants is, indeed, bad. But enum does not make a problem much better. So I would rewrite this part with the following idea. Let us introduce a small class, say, ToolInfo (or ToolDescription) which can hold name of the tool (and configuration name which is needed in case of Bibtex[8u]). Then we will have container of these objects populated manually in the code of from config file. Then all "if"s become enumerations in this container. Just check every object against Biblatex hint. They can have also i18n names and descriptions so actions creation will also be enumeration and signal handling will be direct calls to some dictionary. No more ifs and switches. We will have simple handling of unknown values (if we reached past the enumeration - the value is unsupported). The only one place to check - loading from config. What do You think?

3. Yes, but my problem is that I have very little experience of programming for KDE. I'm reading about KXMLGUI, but still do not understand what did you mean by "There is need to access menus directly".
5. So for now a in-memory storage in LaTeXInfo is OK?

On September 11th, 2012, 12:53 p.m., Michel Ludwig wrote:

1. Ok, it's a good idea to support multiple configurations :) So, let's do it similarly to the live preview menu in "Build" (but still use a KSelectAction)?
The code for the menu is in "LivePreviewManager::buildLivePreviewMenu(KConfig *config)", and it uses hashes to associate actions with tool names and configurations. We can create a new tool class
called "Bibliography" and then populate the KSelectAction with every tool that is based on that class (similar to the "LaTexLivePreview" tool class). You could also try to reuse the ToolConfigPair class that is used there.
For the auto-detection setting, we can invent a reserved tool config name like "KILE_AUTODETECT".
3. All it means that you don't deal with KMenu or QMenu classes, but you only manipulate the KSelectAction. It will be added automatically to the menus by KXMLGUI.
5. Yes, later I will take care of saving it.

On September 11th, 2012, 5:54 p.m., Eugene Shalygin wrote:

1. How I'm supposed to update kilerc file to add "class=Bibliography" to all bib tools? Earlier you have said me that current update system will be abandomed for Kile 3.0. So what should I do?
Also,  I'd like to extend ToolConfigPair to add execuable name. May I? That wil simplify my task and I think can not injure in other places? Or I can make ToolConfigPair a real class and derive BibtoolConfig from it.

On September 11th, 2012, 6:28 p.m., Michel Ludwig wrote:

When running Kile 3 for the first time, the tools will be reloaded, i.e. the tools from the local config file will be overwritten by the tools defined in "kilestdtools.rc". I meant that this reloading could be done in a more intelligent way by not touching user-created tools, for example. You can reset the tools manually in the configuration dialog under Tools / Build / Restore default tools.

ToolConfigPair is a proper class. You could simply declare a new class that derives from it. But is it really necessary? Wouldn't it be sufficient to add a method like

QString commandFor(const QString& tool, const QString& cfg, KConfig *config)
{
return config->group(groupFor(tool, cfg)).readEntry("command", "");
}

to the tool manager? (The tools have pointers to the appropriate config and tool manager objects)

On September 11th, 2012, 7:38 p.m., Eugene Shalygin wrote:

So I just update .rc files. OK. 

ToolConfigPair is a QPair, right? Derived class would have attirbutes "first", "second", and "command". That is not beautiful. And I really would like too have the command loaded since I need it every time I detect backend. Isn't that method "commandFor" too complicated for that? From the other hand, I have to react on tools config changes... But tool names can be changed as well as commands, so reloading is needed anyway.

On September 11th, 2012, 8:16 p.m., Michel Ludwig wrote:

What we can also do is to add a method "ToolConfigPair bibliographyToolFromCommand(const QString& s)" or so to the tool manager. Basically, the tool manager would keep track of the bibliography tool and allow for lookups. Whenever the tools are reloaded, it could update the bibliography tools list as well (but for that you still have to move the "KileTool::Factory::readStandardToolConfig()" method to the tool manager, where it really belongs, and then rebuild the lists)

And the tool manager would also need to listen to the "configChanged" signal from the configuration manager (in "configurationmanager.h").
Thanks! 
Everything is clear except the question with the ToolPairClass. I do not need mappping from tool command to name. I need a list of possible commands binded to tools configurations. If I do not extend TooConfiglPair I'll have a separate list of commands. And a implicit map in bibliographyToolFromCommand(). This is almost the same but less structured. If you are caring about overhead (command is not needed everywhere), we can add parameters to toolsWithConfigurationsBasedOnClass() to will specify which  parts of toll config to load.

P.S. I see that patch becomes big enough. Perhaps I'd better submit it in several parts (like introducing "Bibliography" class, changing ToolConfigPair, adding methods to tool manager, and finally the bibtool selection)?

- Eugene


On September 9th, 2012, 11:02 p.m., Eugene Shalygin wrote:

Review request for Kile.
By Eugene Shalygin.

Updated Sept. 9, 2012, 11:02 p.m.

Description

As we can detect what backend is used by Biblatex only when it prints it out, we need to store that information for next updates of bibliography

This patch use dirty approach: it just stores this as dynamic property of TextInfo object and uses it afterwards. I believe we must store this information somewhere or parse \usepackage{biblatex}.
Maybe in the future Biblatex will provide some other setup commands for specifiyng backend which will be needed to parse also.
I understand, that dynamic property is not the best place to store it. From the other hand, it is kind of local information in this approach, because property name is not used outside.

Testing

Manual testing
Bugs: 268047

Diffs

  • src/documentinfo.h (27ad09f)
  • src/documentinfo.cpp (2c3a3bb)
  • src/kile.cpp (8061b54)
  • src/kileproject.h (e77035a)
  • src/kileproject.cpp (dd4087d)
  • src/kilestdtools.h (769e8ca)
  • src/kilestdtools.cpp (0c6e5f0)
  • src/kiletoolmanager.h (86e2258)
  • src/kiletoolmanager.cpp (d06e36f)
  • src/outputinfo.h (096c708)
  • src/outputinfo.cpp (8d18adc)

View Diff