-
-
Notifications
You must be signed in to change notification settings - Fork 117
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.
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
Eliminated emit of Library class signals from other objects #1131
Conversation
Though the original issue mentions we can connect signals to signals, here I think just using a member function does the job. For this particular case, creating different signals in different classes just for this purpose feels redundant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that these changes are an improvement - the new member functions are no different from the signal being emitted explicitly.
The purpose is to emit signals within the object, which I believe member function would be doing that? For the LibraryManipulator, we cannot inherit QObject so we have to rely on a direct emission or member function of some sort. Emittings signals from public functions are pretty common place. Both creating extra signals and creating member methods are hacks, but they should do the job. @asashnov Do you have an input on whether we should stick to connecting signals as much as possible or is using member function calls fine. |
But doing it via a dedicated member function doesn't make the design better. I guess the design principle is that the objects should emit signals from within otherwise meaningful methods. |
Yeah that is good overall principle, I will refactor this. |
Refactored signals of Library class to be only emitted by itself. Other objects will use member function to emit such signals if they would like.
dccd000
to
10579a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't justify merging these changes as well as continuing the work on this PR. To me it looks like applying a minute amount of Dior fragrance to an athlete after extensive workout before he takes shower and changes his clothes. I believe that the mild issue being addressed should rather be fixed as a by-product of a more serious refactoring/redesign/code-cleanup.
I can agree with that, but then please try to phrase the nature of the problem (and maybe a bit of the solution) within an issue. I understand that this might be a bit challenging, and if too complicated/touchy we won't give it to a student. |
@kelson42 Let's not waste time on bureaucracy around a minor issue. Just assign it to me and I will resolve it over time as I rewrite more of |
Fix #540
Change: