Code review comment for lp:~kaijanmaki/unity8/launcher-backend

Revision history for this message
Michael Zanetti (mzanetti) wrote :

As there were lots of comments and lots of commits, here's a new summary of what's left (and some new things):

===
Icon names not parsed.

===
1164 + Q_FOREACH(QString identifier, m_desktopEntry->actions()) {
1427 + Q_FOREACH(QString appId, tmp) {
1558 + Q_FOREACH(QString id, m_desktopFiles[appId]->desktopEntry()->actions()) {

Use const &

===
Tests
* rename (to testParser) and move test (tests/plugins/...)
* add more tests
* use unity8 cmake makros for declaring test target

===
clean up unused includes (e.g. QDebug)

===
unity8 coding style:
* return values on same line
* api docs in .h instead of .cpp

===
Clean up logging: only print something in real error cases, stick to qWarning instead of logging.h

===
215 +<entry_key>"X-Ubuntu-StageHint" { BEGIN(equal); yyextra->value_type = String; return KEY_UBUNTU_STAGEHINT; }

Clarify if it's a StringList.

===
Parser lacks support for "X-Ubuntu-Touch" of type boolean.

===
flex generated code causes warnings.

===
220 +<entry_key>"StartupWMClass" { BEGIN(equal); yyextra->value_type = String; return KEY_STARTUPVMCLASS; }
754 + | KEY_STARTUPVMCLASS { $$ = KEY_STARTUPVMCLASS; }
1019 +DesktopFileReader::DesktopEntryGroup::startupVMClass() const
(and some more)

should be KEY_STARTUPWMCLASS with W instead of V

===
> 1380 + if (QFile::exists(clickFile)) {
> 1381 + isClick = true;
> 1382 + fileName = clickFile;
> 1383 + } else {
> 1384 + fileName = normalFile;
> 1385 + }

Revert this logic to prefer system apps over click packages.

review: Needs Fixing

« Back to merge proposal