Skip to content

commented out call to QApplication.ProcessEvents() in xcsw_is_visible()#108

Merged
PierreRaybaut merged 4 commits into
masterfrom
107-random-seqfault
Jun 10, 2023
Merged

commented out call to QApplication.ProcessEvents() in xcsw_is_visible()#108
PierreRaybaut merged 4 commits into
masterfrom
107-random-seqfault

Conversation

@RussBerg

Copy link
Copy Markdown
Collaborator

This is a problem I have had for quite a while and only now getting around to submitting an issue and PR for it.
There are two ways I tested to fix this and both worked and neither caused the plot to behave in a way that did not make sense,
first is to comment this line

https://github.com/PierreRaybaut/guiqwt/blob/56ae061e128bcf95e489c347a475ea83dc22fb5a/guiqwt/plot.py#L1251

and the second was to instead comment out the call to ProcessEvents() in xcsw_is_visible,

https://github.com/PierreRaybaut/guiqwt/blob/56ae061e128bcf95e489c347a475ea83dc22fb5a/guiqwt/plot.py#L1304

I chose the later because it is not ideal to call ProcessEvents directly and it didn't cause the plot to behave strangely.

closes #107

@RussBerg
RussBerg requested a review from PierreRaybaut May 16, 2023 16:55
Comment thread guiqwt/plot.py Outdated
def xcsw_is_visible(self, state):
if state:
QApplication.processEvents()
#QApplication.processEvents()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
#QApplication.processEvents()

Just remove the line instead of commenting it.

@RussBerg RussBerg May 16, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok thanks, Truth be told I have been running for months with the first proposed change but opted for what I thought would be the less intrusive option 2 of commenting line 1304 but I just ran into a segfault, unless someone has a better suggestion I am am going to revert back to removing the line that connects SIG_VISIBILITY_CHANGED to the x cross section for this PR

@RussBerg
RussBerg marked this pull request as draft May 16, 2023 17:24
@RussBerg
RussBerg marked this pull request as ready for review May 16, 2023 17:41
@RussBerg
RussBerg requested a review from kalvdans May 16, 2023 17:41
@PierreRaybaut

PierreRaybaut commented May 17, 2023

Copy link
Copy Markdown
Collaborator

Please test the following:

I think that it should avoid all the segmentation faults you are experiencing.
Please tell me if it does.

@PierreRaybaut

Copy link
Copy Markdown
Collaborator

The next step would be the following.
And I think you may directly jump to this solution and ignore my previous comment.

Import QTimer here, adding it next to Qt: https://github.com/PierreRaybaut/guiqwt/blob/56ae061e128bcf95e489c347a475ea83dc22fb5a/guiqwt/plot.py#L120

Replace QApplication.processEvents() by QTimer.singleShot(0, QApplication.processEvents):

@RussBerg
RussBerg removed the request for review from kalvdans May 17, 2023 14:50
@RussBerg

Copy link
Copy Markdown
Collaborator Author

as per your suggestion I tested replacing the direct calls to processEvents() with
QTimer.singleShot(0, QApplication.processEvents) and it failed 3/10 times, I also tried
QTimer.singleShot(10, QApplication.processEvents) and this also failed 3/10 times, I then tried
QTimer.singleShot(100, QApplication.processEvents) and that had 0/10 failures,
left at 100ms I don't think that would really affect the performance of cross section updates right?

If we leave this open for the rest of the week I can keep testing and see if 100 is truly the magic delay

@RussBerg

RussBerg commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator Author

nope the problem remains if using the QTimer solution, the only thing that cures it is to comment out
https://github.com/PierreRaybaut/guiqwt/blob/56ae061e128bcf95e489c347a475ea83dc22fb5a/guiqwt/plot.py#L1251

It seems to not cause any problems as far as I can tell as everything behaves as I would expect.

Comment thread guiqwt/plot.py
@RussBerg
RussBerg marked this pull request as draft June 8, 2023 21:44
@RussBerg
RussBerg marked this pull request as ready for review June 8, 2023 21:46
@RussBerg
RussBerg requested a review from PierreRaybaut June 8, 2023 21:46
Comment thread guiqwt/plot.py
@PierreRaybaut
PierreRaybaut merged commit b40c59f into master Jun 10, 2023
@RussBerg
RussBerg deleted the 107-random-seqfault branch June 12, 2023 14:23
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.

App with multiple ImageWidgets seg faults about 40+% of the time on startup

3 participants