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

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

Thanks for the changes! It looks very good already, but to improve the long-term maintainability/performance of the code, I've added some comments below.

But what do we actually do if two different configurations of a tool have the same command but different parameters otherwise. How do we know which configuration to choose?
Thank you for the time and review!

I can understand your point of view. Agreeing with some of your points, I think that thre is a better solution for others. I reply in particular comments.

Concerning "same commands - different tools" question I belieave that only user can make a desicion and that is why we are introducing user overrides of the autodetection, aren't we?

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kileproject.h (Diff revision 6)
private:
264
	static const QString compilationSettingsConfigGroupName;
I don't think there is a need to declare a class variable for this string.

A preprocessor definition will be enough.
Do I understand correctly that you do not want to pollute the class namespace? We can move the constant to an anonymous namespace in .cpp file.

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kilestdtools.cpp (Diff revision 6)
namespace KileTool
337
		static const ToolConfigPair defaultTool = ToolConfigPair(QString("BibTeX"), QString());
There's no need to declare a variable here.
I disagree. Having static variable we can return it more efficiently comparing to creation of QStrings(). Moreover, declaration can be moved in the function body and be used as a return value some in some additional if-branch if (if we need to change the algorithm in that way).

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kiletool.h (Diff revision 6)
class KConfig;
39
	QString compactToolName(const ToolConfigPair& tool);
These methods should no longer be required.
No, it is needed for action name construction.

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kiletoolmanager.h (Diff revision 6)
namespace KileTool
121
		const CommandToolMap& bibliographyTools() const;
Instead of exposing the map, could you use methods to access it instead? like "containsBibliographyTool" or "findTool...".
The code will be easier to maintain that way.
I doubt how to do that. Iteration over tools is needed. If we have containsBibliographyTool() and findTool() that means we perform search in the map tools several times instead of iteration. What would help is forEachTool(Predicate, Action). Do you want it?

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kiletoolmanager.h (Diff revision 6)
namespace KileTool
178
		typedef QMap<QString, KAction*>	BibliographyActionsMap;
Can you use a QHash instead of a QMap? This is faster.
Not necessarily: with our 3 and a half bib tools, string comparison can not be slower than calculation of a hash value from a key. I do not think that we will have hudreds of bib tools.

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/kiletoolmanager.cpp (Diff revision 6)
namespace KileTool
103
		m_bibliographyBackend(0)
Can you use "NULL" instead of "0"?
Maybe, even nullptr?

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/outputinfo.h (Diff revision 6)
class LatexOutputInfo : public OutputInfo
148
		void setBibliographyBackendToolNameUserOverride(const QString& toolName);
Can you make it take "const ToolConfigPair& p" as argument? In that way we don't have to convert back and forth between a string and a ToolConfigPair.
To this and the next: yes, it would be nice, but for doing that one has to move ToolConfigPair declaration out of "kiletool.h" which includes "outputinfo.h" to avoid circular includes. Where can I put it?

On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/outputinfo.h (Diff revision 6)
class LatexOutputInfo : public OutputInfo
149
		QString bibliographyBackendToolNameUserOverride() const;
Can you make it return a ToolConfigPair?


On September 14th, 2012, 8:30 p.m., Michel Ludwig wrote:

src/widgets/toolconfigwidget.cpp (Diff revision 6)
namespace KileWidget
313
		m_manager->reloadToolsMap();
Can you connect this slot to the "configChanged()" signal from the configuration manager ("configurationmanager.h") instead of calling it directly?
Thank you, done. I did not know about this signal.

- Eugene


On September 14th, 2012, 10:37 a.m., Eugene Shalygin wrote:

Review request for Kile.
By Eugene Shalygin.

Updated Sept. 14, 2012, 10:37 a.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/data/kilestdtools-win.rc (85e70d4)
  • src/data/kilestdtools.rc (765a604)
  • 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/kiletool.h (2b6c035)
  • src/kiletool.cpp (4e2fe8d)
  • src/kiletoolmanager.h (86e2258)
  • src/kiletoolmanager.cpp (d06e36f)
  • src/kileui.rc (e9f2b92)
  • src/outputinfo.h (096c708)
  • src/outputinfo.cpp (8d18adc)
  • src/widgets/toolconfigwidget.cpp (1845291)

View Diff