Codebase: emiting a signal forcedly from outside is a bad practice

[asashnov]

kiwix-desktop 'master' (830e19b)

Found during working on #454

From src/zimview.cpp:

emit mp_tabBar->currentTitleChanged(str);
emit mp_tabBar->currentZimIdChanged(zimId);
emit mp_tabBar->webActionEnabledChanged(QWebEnginePage::Back, mp_webView->isWebActionEnabled(QWebEnginePage::Back));
emit mp_tabBar->webActionEnabledChanged(QWebEnginePage::Forward, mp_webView->isWebActionEnabled(QWebEnginePage::Forward));
emit mp_library->bookmarksChanged();

According to Qt documentation, this is a bad practice.
By signals, only the object itself should notify that its internal state has changed:

https://doc.qt.io/qt-5/signalsandslots.html

Signals
Signals are emitted by an object when its internal state has changed in some way that might be interesting to the object's client or owner. Signals are public access functions and can be emitted from anywhere, but we recommend to only emit them from the class that defines the signal and its subclasses.

[kelson42]

@asashnov Fell free to propose an alternative if this helps to fix #454 or trivial to do.

[kelson42]

What could be an alternative approach?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[asashnov]

Good approach is suggested by Qt documentation:

Signals are emitted by an object when its internal state has changed

we recommend to only emit them from the class that defines the signal

It is even possible to connect a signal directly to another signal

All classes that inherit from QObject or one of its subclasses (e.g., QWidget) can contain signals and slots. Signals are emitted by objects when they change their state in a way that may be interesting to other objects. This is all the object does to communicate. It does not know or care whether anything is receiving the signals it emits. This is true information encapsulation, and ensures that the object can be used as a software component.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[kelson42]

@asashnov I rechallenge you on this: what is left to do?

[asashnov]

kelson42@ Part3 would be illiminating emit mp_library->bookmarksChanged();

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[kelson42]

@shaopengLinCan we move forward on this issue while PR currently in review waits for the reviewer? Do you have an opinion or a proposal on this issue?

[ShaopengLin]

So the mp_library emits are needed because they were directly using the kiwix library that don't emit signals. I will do a PR soon and design decisions can happen there.

[kelson42]

@ShaopengLin Thank you. I have to admit that the rational behind this issue are not clear to me. So, trust you to understand it :)

[kelson42]

@veloman-yunkan Like proposed, can you please quickly implement the fix?

[veloman-yunkan]

@kelson42 That is absolutely not a priority for me now. Let's first release 2.4.0.