(release_30)refactor: eliminate duplicated set of pointers in dlgTriggerEditor class#352
Merged
SlySven merged 1 commit intoMudlet:release_30from Dec 4, 2016
Conversation
Whilst looking at a different issue I spotted that there were two sets of
pointers in the dlgTriggerEditor class that were intended to track the
"current" item in each of Aliases, Buttons ("Actions"), Keys, Scripts,
Timers, Triggers and Variables. The more appropriately named
mpCurrentXXXXItem ones were largely unused and the mCurrentXXXX ones were
mainly used. However in the case of the ones for Variables the mixing of
the use of both of them, may, I thought, have resulted in some oddities in
hiding and un-hiding of variables that I previously remembered happening.
In fact it doesn't but that is another story...
Also:
* Found some systematic duplicate checks in code for Aliases, Keys etc.
for a QTreeWidgetItem pointer being non-null which I was able to remove.
I then spotted a similar test in some Variable handling code in:
(void) dlgTriggerEditor::slot_var_selected(QTreeWidgetItem *)
but there were several calls to other methods which used the pointer as
an argument and it was not immediately clear whether they would modify
the pointer value or it's pointee. Further investigation enable me to
add some const qualifiers to those methods which revealed that the
address stored in the pointer was not going to be changed so that the
second test in the method was redundent.
* There was some unneeded C-Style casts in this class and in the cases
where a cast was needed to resolve signature ambiguities for the compiler
it is better to use a static_cast in C++ code.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Member
|
@Chris7 mind having a look? |
SlySven
commented
Nov 23, 2016
| QStringList sL; | ||
| sL << "Triggers"; | ||
| mpTriggerBaseItem = new QTreeWidgetItem( (QTreeWidgetItem*)0, sL ); | ||
| mpTriggerBaseItem = new QTreeWidgetItem( static_cast<QTreeWidgetItem *>(0), sL ); |
Member
Author
There was a problem hiding this comment.
The cast is needed here and in other similar calls for other elements so the right constructor gets used...
Chris7
reviewed
Dec 1, 2016
| QTreeWidgetItem * pItem = (QTreeWidgetItem*)mCurrentVar; | ||
| if (!pItem || !pItem->parent() ) | ||
| QTreeWidgetItem * pItem = mpCurrentVarItem; | ||
| if (!pItem->parent()) |
Member
Author
There was a problem hiding this comment.
Oh, good catch!; it might be OK, but lets assume things can go wrong or the user can do things that we as developers don't consider reasonable!
I'll mark the PR as broken until I've tweaked this...
We've already checked the member variable (now mpCurrentVarItem; was mCurrentVar) in line (that is now 3941; was 3949) before we assign it's value to the local scope variable pItem...!
Member
|
Looks like a good increase in code clarity, and in a part that if something was done wrong it'd probably seg fault pretty fast. I always hated the mXXX/mpXXX duplication so I'm glad to see that gone. I haven't tested it but I trust @SlySven actually played with the variables to make sure they still work. |
Member
Author
|
False alarm - it wasn't broken after all! 😀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Whilst looking at a different issue I spotted that there were two sets of pointers in the
dlgTriggerEditorclass that were intended to track the "current" item in each of Aliases, Buttons ("Actions"), Keys, Scripts, Timers, Triggers and Variables. The more appropriately namedmpCurrentXXXXItemones were largely unused and themCurrentXXXXones were mainly used. However in the case of the ones for Variables the mixing of the use of both of them, may, I thought, have resulted in some oddities in hiding and un-hiding of variables that I previously remembered happening.In fact it doesn't but that is another story...
Also:
Found some systematic duplicate checks in code for Aliases, Keys etc. for a
QTreeWidgetItempointer being non-null which I was able to remove. I then spotted a similar test in some Variable handling code in:(void) dlgTriggerEditor::slot_var_selected(QTreeWidgetItem *)but there were several calls to other methods which used the pointer as an argument and it was not immediately clear whether they would modify the pointer value or it's pointee. Further investigation enable me to add some
constqualifiers to those methods which revealed that the address stored in the pointer was not going to be changed so that the second test in the method was redundant.There was some unneeded C-Style casts in this class and in the cases where a cast was needed to resolve signature ambiguities for the compiler it is better to use a
static_castin C++ code.Signed-off-by: Stephen Lyons slysven@virginmedia.com