Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Part 2 to fix #540: don't emit tabbar signal from zimview #751

Merged
merged 6 commits into from
Jan 31, 2022
Merged

Part 2 to fix #540: don't emit tabbar signal from zimview #751

merged 6 commits into from
Jan 31, 2022

Conversation

asashnov
Copy link
Contributor

@asashnov asashnov commented Dec 20, 2021 •
edited
Loading

Partial fix #540: stop emitting TabBar::webActionEnabledChanged from ZimView
as it's bad practice according to QObject documentation.

Dismiss way-round signal passing through KiwixApp::currentTitleChanged
Now it's local to MainWindow and its siblings (TabBar, ZimViews, TopWidget).

TabBar, TopWidget, SideBar- connect signals in MainWindow constructor
after MainWindow mp_ui->setupUi() completed.

@asashnov asashnov marked this pull request as draft December 20, 2021 10:10
@asashnov asashnov requested a review from mgautierfr December 21, 2021 17:39
@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch from 0e5d241 to 5496ba5 Compare December 29, 2021 05:57
@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch from 5496ba5 to 863c10d Compare January 7, 2022 07:51
@kelson42
Copy link
Collaborator

@asashnov Can you please rebase and remove the draft?

@asashnov asashnov marked this pull request as ready for review January 13, 2022 13:49
@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch 2 times, most recently from 6e455ac to 6465f5d Compare January 18, 2022 09:17
@kelson42 kelson42 requested review from veloman-yunkan and removed request for mgautierfr January 19, 2022 15:52
@kelson42
Copy link
Collaborator

@veloman-yunkan Would you be able please to review this PR, unfortunately @mgautierfr is super busy these days.

veloman-yunkan
veloman-yunkan reviewed Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set of changes in this PR is not so easy to understand and validate when viewed as one commit. I think that this PR can - and therefore must - be split into several smaller commits (with explanations in the commit messages where appropriate).

@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch from 6465f5d to ec4f2ac Compare January 26, 2022 11:41
veloman-yunkan
veloman-yunkan requested changes Jan 26, 2022
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some commit titles exceed the recommended length limit and look ugly when split with an ellipsis

Looks like the commit titled '#540: don't emit TabBar::webActionEnabledChanged signal from ZimView' can be split further into two more-or-less independent changes.

src/tabbar.h Outdated Show resolved Hide resolved
src/mainwindow.cpp Show resolved Hide resolved
@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch from ec4f2ac to 150593b Compare January 28, 2022 19:40
@asashnov
Copy link
Contributor Author

asashnov commented Jan 28, 2022 •
edited
Loading

Some commit titles exceed the recommended length limit and look ugly when split with an ellipsis

Fixed. Thanks for pointing this out. My further commits will be better from start.

Looks like the commit titled '#540: don't emit TabBar::webActionEnabledChanged signal from ZimView' can be split further into two more-or-less independent changes.

Fixed. Split changes to commits reworked.

@asashnov
Drop redundant Kiwixapp::mp_tabWidget pointer
5e7b856 
No need to story what can be easily get.
@asashnov
Make 'TabBar::onCurrentChanged()' slot private
7347086
@asashnov
Don't emit TabBar's signal from outside
bba53bc 
According to Qt QObject manual only an QObject itself should emit
their signal, indicating its internal state has changed.
Calling emit on others' signal is a bad practice.

References: #540
@asashnov
Drop extra '#include'
60571e6
@asashnov
Drop 'KiwixApp::currentTitleChanged()' signal
fd90eb8 
When TabBar emits currentTitleChanged(), its only affects
other children of MainWindow: TopWidget and SearchBar.

So it makes the code more manageble if this knowledge
is local to MainWindow only.
@asashnov
Localize MainWindow inner-children connections
921615c 
Move 'connect()' call from KiwixApp to MainWindow
because MainWindow should be responsible for connecting
its children between each other.
@asashnov asashnov force-pushed the 540-part2-dont-emit-tabbar-signal-from-zimview branch from 150593b to 921615c Compare January 28, 2022 19:50
veloman-yunkan
veloman-yunkan reviewed Jan 29, 2022
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one question. If the answer to it is negative then this is an approval.

src/mainwindow.cpp Show resolved Hide resolved
@kelson42 kelson42 merged commit fe01853 into master Jan 31, 2022
@kelson42 kelson42 deleted the 540-part2-dont-emit-tabbar-signal-from-zimview branch January 31, 2022 08:07
@kelson42 kelson42 mentioned this pull request Feb 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veloman-yunkan veloman-yunkan veloman-yunkan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Codebase: emiting a signal forcedly from outside is a bad practice
3 participants
@asashnov @kelson42 @veloman-yunkan