Core: Fix various Qt warnings #216
Conversation
src/core/field/Opcode.h
Outdated
| class Opcode | ||
| class Opcode : public QObject | ||
| { | ||
| Q_OBJECT |
There was a problem hiding this comment.
See the main PR description about this part
There was a problem hiding this comment.
I don't want to use this macro, as it adds too much overhead, I only need a tr() function
There was a problem hiding this comment.
lupate is the only one to complain in its code analysis
There was a problem hiding this comment.
Ok shall I revert this commit then? The main issue is that the compiler says the class needs Q_OBJECT defined. The whole warning starts from this line of code: https://github.com/julianxhokaxhiu/makoureactor/blob/master/src/core/field/Opcode.cpp#L2093
There was a problem hiding this comment.
Yes, revert please. Like I said, that's not a compiler warning, but a lupdate warning, for the wrong reasons
There was a problem hiding this comment.
Ok dropped the patch entirely, thank you!
There was a problem hiding this comment.
About this file
- Do Not include a module like is done in this class with
#include <QtCore>. Instead only include the needed objects in this case QObject and QString . This will reduce compile time The entire project should be checked for this . - Never use the Q_OBJECT macro in a class that is not subclased from QObject. If you need to you can use
Q_GADGETbut for this class just callQObject::tr. and then there is not a need for moc so no reason to have it run on the object.
Co-authored-by: Jérôme Arzel <myst6re@gmail.com>
julianxhokaxhiu
force-pushed
the
master
branch
from
2ec4f21 to
0803aef
Compare
|
|
||
| actionOpen = fileMenu->addAction(QIcon::fromTheme(QStringLiteral("document-open")), tr("&Open..."), this, [&] { openFile(QString());}, QKeySequence("Ctrl+O")); | ||
| fileMenu->addAction(QIcon::fromTheme(QStringLiteral("document-open-folder")), tr("Open &Directory..."), this, &Window::openDir, QKeySequence("Shift+Ctrl+O")); | ||
| actionOpen = fileMenu->addAction(QIcon::fromTheme(QStringLiteral("document-open")), tr("&Open..."), QKeySequence("Ctrl+O"), this, [&] { openFile(QString());}); |
There was a problem hiding this comment.
Consider using a standard QKeySequence such as QKeySequence::Open using this is prefred as it will use the OS shortcut (and in the case of a default being combo being changed it will use that )
There was a problem hiding this comment.
If you bump to Require Qt 6.7+ (I would do 6.8 as it LTS) you can also do the same with alot of the icons too. by using https://doc.qt.io/qt-6/qicon.html#ThemeIcon-enum
There was a problem hiding this comment.
Consider using a standard QKeySequence such as
QKeySequence::Openusing this is prefred as it will use the OS shortcut (and in the case of a default being combo being changed it will use that )
Could lead to conflicts between shortcuts, for example on mac os, shortcuts are often different comparing to Windows, if I have a non standard shortcut (for example to show a specific dialog), I need to ensure that it is not used in any platform I want to support to avoid conflicts
There was a problem hiding this comment.
? Open should have use the QKeySequence::Open shortcut . ON mac os it will be cmd+O and windows crtl+O if yoru app has focus the shortcut will go to your app (first to the widgets w/ focus) . So not really sure what your trying to say they can be an issue for.
There was a problem hiding this comment.
Imagine on the next Qt major update, QKeySequence::Open is now mapped to Ctrl+P on Mac, but Makou already use this shortcut elsewhere, you will have a undetected conflict.
But okay, this example will never happen
There was a problem hiding this comment.
Its weird some of them may not be registered on some oses . so where you can i use them but yeah in general i just sucks hotkeys..
3 participants
This PR makes the code warning free.
However please check this separate commit 06c98f3 as while it fixes technically the problem, and it seems to not create any regression ( I tested Makou and it seems to be working as usual ), I'm not sure if this creates a separate problem as I'm no Qt expert.Solved, see conversation below.As usual artifacts are in the PR or my own own action result: https://github.com/julianxhokaxhiu/makoureactor/actions/runs/17008837818