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.

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

Revert "Enabled keyboard navigation for the trigger editor" #60

Merged
merged 2 commits into from Aug 19, 2014
Merged

Revert "Enabled keyboard navigation for the trigger editor" #60

merged 2 commits into from Aug 19, 2014

Conversation

ahmedcharles
Copy link
Contributor

This reverts commit ce10a63.

Conflicts:
src/dlgTriggerEditor.cpp
src/dlgTriggerEditor.h

@SlySven
Copy link
Member

SlySven commented Aug 18, 2014

You may wish to note that for all those itemClicked() signals on the treeWidget items if a doubleClick occurs then the doubleClicked() signal is emitted THEN a second itemClicked() is produced - if the itemClicked() signal handler does anything to alter the current editor state - including the contents of the "script" area - then repeating the action may not be a good idea! I did consider having a "dlgTriggerEditor"-wide isProcessingDoubleClick flag and then setting it in the doubleClick handler so that it could cause an early bail out of the itemClick handler (after resetting the flag) to avoid this sort of effect.

@ahmedcharles
Copy link
Contributor Author

Is that an existing issue or a regression?

This is a fix for a regression in behavior, if double click never worked, then it's not a regression, that's just a bug.

@SlySven
Copy link
Member

SlySven commented Aug 18, 2014

That was an existing "feature" that might touch upon some part of the editor problems and since your reversion will be putting back those itemclicked() signals it will return to possibly be a factor - I do not fully understand the program flow in that part of Mudlet (I have a faint suspicion that I'm not the only one in that state 😉)

@ahmedcharles
Copy link
Contributor Author

This fixes the bug without any downside and I think I understand why the other approach fails, but it will require more research. It's related to the fact that addTrigger calls treeWidget->setCurrentItem( pNewItem ); which implies an item changed event, which calls the slot, which saves the trigger when things are in the wrong state.

@vadi2
Copy link
Member

vadi2 commented Aug 19, 2014

Yeah :(

ahmedcharles added a commit that referenced this pull request Aug 19, 2014
Revert "Enabled keyboard navigation for the trigger editor"
@ahmedcharles ahmedcharles merged commit 70f186e into Mudlet:development Aug 19, 2014
@ahmedcharles ahmedcharles deleted the bug_fix branch August 19, 2014 10:46
@vadi2
Copy link
Member

vadi2 commented Aug 19, 2014

Should've waited until the other pull request is ironed out.

On Tue, Aug 19, 2014 at 8:46 PM, Ahmed Charles notifications@github.com
wrote:

Merged #60 #60.


Reply to this email directly or view it on GitHub
#60 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants