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

src/kileproject.h (Diff revision 6)
private:
264
	static const QString compilationSettingsConfigGroupName;
Above all, I don't want to waste memory. If you declare a class variable, it will end up in the executable, unnecessarily occupying a few bytes. All you need it for is to define some string locally in KileProject. Hence, something like

#define SETTINGS_GROUP_NAME "bla"

will be enough, and no variable declaration is needed.

src/kilestdtools.cpp (Diff revision 6)
namespace KileTool
337
		static const ToolConfigPair defaultTool = ToolConfigPair(QString("BibTeX"), QString());
What you probably meant was return a reference to a ToolConfigPair (ToolConfigPair&). Unlike Java, C++ does not consider every object variable to be a reference. 

Here, if you don't put "static", the performance will be exactly the same as most likely the variable will be optimised away by the compiler.

With "static", the variable won't end up on the stack but in a different location in the executable, which probably means that it will have to be loaded whenever it is accessed, and causing a slight slowdown.

If you absolutely want a variable there, then I wouldn't declare it as "static".

src/kiletool.h (Diff revision 6)
class KConfig;
39
	QString compactToolName(const ToolConfigPair& tool);
Could you use the same code as in "LivePreviewManager::buildLivePreviewMenu"? (which actually isn't right as there should be a call to i18n when the text of the action is constructed)

The texts of the tool selection actions should be the same in the live preview submenu and the bibliography submenu.

src/kiletoolmanager.h (Diff revision 6)
namespace KileTool
121
		const CommandToolMap& bibliographyTools() const;
Hehe, what I ideally want are a few methods with self-explanatory names that make it clear what they are supposed to do. With exposing the map, I will probably look at the code again in a few months/years and it will take me some time to figure out what's going on ;)

There's no need to worry about performance here as we are only dealing with a handful of tools anyway. The need for maintainability of the code outweighs any performance gains here.

src/kiletoolmanager.h (Diff revision 6)
namespace KileTool
178
		typedef QMap<QString, KAction*>	BibliographyActionsMap;
The Qt documentation states that "QHash provides faster lookups than QMap". QMap will also compute hash values of strings, but it also provides some ordering guarantee on the keys, which we don't really need.

I agree that there probably won't be any performance difference, but it is nevertheless a good habit to use the most appropriate classes everywhere. In this case that would be QHash.

src/kiletoolmanager.cpp (Diff revision 6)
namespace KileTool
103
		m_bibliographyBackend(0)
nullptr is part of C++11, which is not yet supported by all of the compilers that are supposed to be able to compile KDE software. So, we cannot use it yet.

- Michel


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