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.

Sign up for GitHub

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

Jump to bottom

[IDE] Fix detached docklet visibility bug #3660

Merged
merged 1 commit into from
Apr 12, 2018
Merged

[IDE] Fix detached docklet visibility bug #3660

merged 1 commit into from
Apr 12, 2018

Conversation

gusano
Copy link
Member

@gusano gusano commented Apr 10, 2018

Fix #3287

This fixes what seems to be a weird race condition: when starting the IDE, a detached (and closed) docklet container can be raised just after it has been hidden, leading into a ghost unresponsive window (see linked issue).

I'm not sure this is the most elegant fix so I'm open to any hint if this needs to be done differently.

cc @jamshark70

gusano
gusano commented Apr 10, 2018
View reviewed changes
editors/sc-ide/widgets/main_window.cpp Outdated
@@ -67,6 +67,7 @@
#include <QUrl>
#include <QMimeData>
#include <QMetaMethod>
#include <QDebug>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not directly related but it was missing.. Let me know if I should revert that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you included it for qDebug(), it's not really necessary as qDebug is a global feature: http://doc.qt.io/qt-5/qtglobal.html#qDebug

mossheim
mossheim reviewed Apr 11, 2018
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks for this! i can confirm the behavior is OK on macOS. i never was able to reproduce this bug though, so take it with a grain of salt.

editors/sc-ide/widgets/util/docklet.hpp Outdated
@@ -74,7 +74,7 @@ class Docklet : public QObject
return const_cast<Docklet*>(this)->currentContainer() != mDockWidget;
}

void setDetached( bool detached );
void setDetached( bool detached, bool visible = true );
Copy link
Contributor

Choose a reason for hiding this comment

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

consider renaming this function; a call to "setDetached( bool, bool )" is opaque. suggestion - setDetachedWithVisibility. i don't think a default value for the second argument is a good idea, it will make calling code harder to understand.

@jamshark70
Copy link
Contributor

I've just launched the IDE five times in a row, without a single ghost help browser appearing.

I'm crying.

Thank you @gusano !!!

I don't have much to say about code review. Sometime later I might suggest including some of the changes I was testing, specifically to defer MainWindow initialization by a QTimer. The bulk of it is here -- jamshark70@1cd6643 -- but I would have to check if I added something functionally different in another branch.

The reason is: I also notice that the main IDE window "drifts" to the left when launching. I usually have the IDE window flush left on the screen, but often when I launch the IDE, the window's left margin is slightly offscreen. I found, when I was testing the suggestion from the Qt forum, that the window position was more accurate.

Using this PR as is, I still get the window-position problem.

I cherry picked the above commit and tested, but then it always shows the help browser. I think it's because of help_browser.hpp:

    void onInterpreterStart() {
        if (isVisible() && mHelpBrowser->url().isEmpty())
            mHelpBrowser->goHome();
    }

... where isVisible() is true, even if the window is supposed to be hidden.

But, come to think of it, window drift is another issue to log later.

I'd like to see this fix in 3.9 (currently based on develop).

@jamshark70
Copy link
Contributor

FWIW ecd59e1 cherry-picks cleanly onto 3.9, tested locally and it's good.

@mossheim
Copy link
Contributor

@jamshark70 - we aren't planning to do any more 3.9 releases (this has been mentioned a few times on the ML). if 3.10 takes longer than expected maybe we can do another patch release and backport some of the fixes

@gusano
ide - docklet: fix detached docklet visibility
dd1b8a8 
Signed-off-by: Yvan Volochine <yvan.github@gmail.com>
@gusano gusano force-pushed the topic/detached-docklet-visibility branch from a87e634 to dd1b8a8 Compare April 11, 2018 12:39
@gusano
Copy link
Member Author

gusano commented Apr 11, 2018

@brianlheim thanks! Force pushed with your suggestions.
@jamshark70 actually the first reason that made me have a look at this issue was that using QTimer sounded wrong to me :) Let's fix one bug at a time 👹

@jamshark70
Copy link
Contributor

I agree this PR should fix just the one bug.

If QTimer is wrong, then I'm not clear why the IDE window is positioned incorrectly without it ;) but that's a question for another thread.

mossheim
mossheim approved these changes Apr 11, 2018
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

I can confirm this doesn't have any strange effects on Windows (at least none I can observe). I see no reason not to merge this.

@gusano gusano merged commit 42134ea into supercollider:develop Apr 12, 2018
@gusano gusano deleted the topic/detached-docklet-visibility branch April 12, 2018 08:03
@nhthn nhthn added this to the 3.10 milestone Apr 12, 2018
This was referenced Jun 4, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

Assignees
No one assigned
Labels
env: SCIDE
Projects
None yet
Milestone
3.10
Development

Successfully merging this pull request may close these issues.

Detached, closed docklet partially reappears when launching the IDE
4 participants
@gusano @jamshark70 @mossheim @nhthn