Skip to content

Fix problems with Composer window closing#3386

Merged
vadi2 merged 4 commits intoMudlet:developmentfrom
atari2600tim:fixComposerCrash
Mar 13, 2020
Merged

Fix problems with Composer window closing#3386
vadi2 merged 4 commits intoMudlet:developmentfrom
atari2600tim:fixComposerCrash

Conversation

@atari2600tim
Copy link
Copy Markdown
Contributor

@atari2600tim atari2600tim commented Mar 7, 2020

Brief overview of PR changes/additions

  • Added closeEvent to Composer window which clears pointer to itself in cTelnet when it gets closed other than using save/cancel button (such as clicking X or right-click and close), so that the parent object will know that it is gone
  • Added to cTelnet destructor saying to close Composer window, so that there won't be an orphaned composer window

Motivation for adding to Mudlet

Allows you to open an editor window again after closing an earlier editor window by clicking the X instead of cancel/save. Avoids segfault that currently happens if you click cancel on the composer window after closing the profile that it was associated with.

Other info (issues closed, discussion etc)

This should solve issue #3300
In addition to not being able to open a new editor after closing it, there is also problem where if you leave the composer window open and close your profile then the window is still there and crashes mudlet if you push button to save or cancel, should solve that too.

@atari2600tim
Merge pull request #3 from Mudlet/development
cc91173 
Fix createLabel (..., true) (Mudlet#3384)
@atari2600tim
fix segfault with Composer window being left open
ecc06f4 
* Added closeEvent to Composer window which clears pointer to itself in cTelnet when it gets closed without pushing save/cancel (such as clicking X or right-click and close)
* Added to cTelnet destructor saying to close Composer window
@atari2600tim atari2600tim requested a review from a team as a code owner March 7, 2020 17:16
@atari2600tim atari2600tim requested a review from a team March 7, 2020 17:16
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Mar 7, 2020 •
edited
Loading

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 7, 2020

👍 fixes look good to me - will test when I can. Not enough time to make it into this release with testing, but it should be good for the next one.

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Mar 8, 2020

Can I make the observation that a quicker and simpler method would be to change the line in ctelnet.h from:

    dlgComposer* mpComposer;

to:

    QPointer<dlgComposer> mpComposer;

We already do that for some other dialogue pointers - for the same reason...! If you do that instead then you do not need any code in dlgComposer.cpp - you can also (independently) change the code in the ctelnet destructor from a mpComposer::close() call to a mpComposer::deleteLater() - which behaves better if it should be called more than once on the same object {it defers the destruction briefly, until it is convenient in the main GUI event loop}, the Qt Documentation says of QObject::deleteLater():

Note: It is safe to call this function more than once; when the first deferred deletion event is delivered, any pending events for the object are removed from the event queue.

@atari2600tim
Use QPointer and set flag to delete on close (#8)
528f7d5 
* Update dlgComposer.h

* Update dlgComposer.cpp

* Update ctelnet.cpp

* Update ctelnet.h

* Update dlgComposer.cpp
@atari2600tim
missed part (#9)
b56bb97 
* Update ctelnet.cpp

* Update ctelnet.cpp
@atari2600tim
Copy link
Copy Markdown
Contributor Author

atari2600tim commented Mar 9, 2020 •
edited
Loading

That all sounds good.

It took me a while to figure out why that didn't work. Turns out the close function was not getting rid of it and needed a flag to do so.
The widget is hidden if it accepts the close event
If the widget has the Qt::WA_DeleteOnClose flag, the widget is also deleted
So I guess this now also fixes a memory leak in the cancel/save button code that I originally had repeated for the X button.

Now I'm able to close it and open it again, and also close the profile with it open with no problem.

vadi2
vadi2 approved these changes Mar 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Works well in testing! Will leave to @SlySven to review too.

@vadi2 vadi2 merged commit 9533c79 into Mudlet:development Mar 13, 2020
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 13, 2020

Thanks a lot for the fix, @atari2600tim! Looking forward to more :)

@atari2600tim atari2600tim mentioned this pull request May 12, 2020
Closed
@atari2600tim atari2600tim deleted the fixComposerCrash branch March 13, 2021 17:23
Chris7 pushed a commit to Chris7/Mudlet that referenced this pull request Jan 2, 2022
@atari2600tim @Chris7
Fix problems with Composer window closing (Mudlet#3386)
98e06cb 
* fix segfault with Composer window being left open

* Added closeEvent to Composer window which clears pointer to itself in cTelnet when it gets closed without pushing save/cancel (such as clicking X or right-click and close)
* Added to cTelnet destructor saying to close Composer window

* Use QPointer and set flag to delete on close (Mudlet#8)

* Update dlgComposer.h

* Update dlgComposer.cpp

* Update ctelnet.cpp

* Update ctelnet.h

* Update dlgComposer.cpp

* missed part (Mudlet#9)

* Update ctelnet.cpp

* Update ctelnet.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vadi2 vadi2 vadi2 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.

3 participants

@atari2600tim @vadi2 @SlySven