undocking/detaching toolviews to regular windows instead of tool windows

[René J.V. Bertin]

Hi,

Anyone else who'd like to be able to undock/detach toolview windows to become regular windows that remain visible when application focus changes, that can be covered by the mainwindow (and moved partially off screen)? The documentation toolview would be a prime candidate in my book.

FWIW I'm making some progress but could use a hand esp. from someone understanding the architecture a bit better - if such a feature would be appreciate by others than just myself.

Thanks,
René

[Leon Pollak]

Post by René J.V. Bertin
Hi,
Anyone else who'd like to be able to undock/detach toolview windows to
become regular windows that remain visible when application focus changes,
that can be covered by the mainwindow (and moved partially off screen)? The
documentation toolview would be a prime candidate in my book.
FWIW I'm making some progress but could use a hand esp. from someone
understanding the architecture a bit better - if such a feature would be
appreciate by others than just myself.
Thanks,
René

In my humble opinion it will be useful in the case you mentioned...

[René J.V. Bertin]

Post by Leon Pollak
In my humble opinion it will be useful in the case you mentioned...

Leon, if you (or anyone else) wants to have a preview:

adds standalone window support to IdealDockWidget:
https://github.com/RJVB/macstrop/blob/master/kf5/kf5-kdevelop/files/devel52/patch-regular-undocked-windows.diff

makes the documentation toolview use standalone windows (hardwired):
https://github.com/RJVB/macstrop/blob/master/kf5/kf5-kdevelop/files/devel52/patch-docview-standalone.diff


These are for the 5.2 branch, and they intersect somewhat with my QTextBrowser-docview patches so let me know if they don't work out because of that or any other reason.

I'll be testing them myself some more and put them up for review when I feel like it.

R.

[René J.V. Bertin]

I've been working on a complement/alternative to using the embedded documentation toolview with standalone windows. Those are nice, but what I have really been hoping for is a (QtHelp) documentation browser like Qt's Assistant (the one in Qt Creator is even nicer but not standalone).

This patch gives me such a feature: https://phabricator.kde.org/D4484 . With that in place, I don't even have to add the Documentation toolview to a toolbar. Clicking on the "Show documentation for" link in help context popups will show the documentation in question in the Assistant.

Last paragraph in the latest update message of D4484:
As argued before, I see this as a good complement to an embedded documentation viewer that's based on QTextBrowser, a step beyond what's possible with a floating embedded doc viewer, even if it uses regular windows. Qt's Assistant supports having multiple tabs open for instance, has better search and filtering facilities, and can host a centralised (shared) collection of QHC documentation from various locations, accessible without having to start KDevelop (or add them in KDevelop too).

[Kevin Funk]

Post by René J.V. Bertin
Hi,
Anyone else who'd like to be able to undock/detach toolview windows to
become regular windows that remain visible when application focus changes,
that can be covered by the mainwindow (and moved partially off screen)? The
documentation toolview would be a prime candidate in my book.

Makes sense. Patches welcome.

Should be easy enough, you need to reparent the widget contained in the
QDockWidget, set the Qt::Window flag and show it again. Work from there.

Something I hacked together in 5 mins (basically untested):

```
diff --git a/kdevplatform/sublime/idealdockwidget.cpp b/kdevplatform/sublime/
idealdockwidget.cpp
index d8fcac4be9..31541dd21d 100644
--- a/kdevplatform/sublime/idealdockwidget.cpp
+++ b/kdevplatform/sublime/idealdockwidget.cpp
@@ -140,6 +141,16 @@ void IdealDockWidget::contextMenuRequested(const QPoint
&point)
setShortcut->setToolTip(i18n("Use this shortcut to trigger visibility of
the tool view."));

menu.addSeparator();
+ QAction *separateWindow = menu.addAction(i18n("Turn into separate
window"));
+ connect(separateWindow, &QAction::triggered, this, [this]() {
+ if (auto w = widget()) {
+ // turn into top-level window
+ w->setParent(nullptr);
+ w->setWindowFlag(Qt::Window, true);
+ w->show();
+ });
+
+ menu.addSeparator();
QAction* remove = menu.addAction(QIcon::fromTheme(QStringLiteral("dialog-
close")), i18n("Remove Tool View"));

QAction* triggered = menu.exec(senderWidget->mapToGlobal(point));
```

Seems to do the job. But please investigate some more yourself, please make
sure that doesn't break any other integration within the Sublime architecture.
Only then file a patch, best with some unit test.

Regards,
Kevin

Post by René J.V. Bertin
FWIW I'm making some progress but could use a hand esp. from someone
understanding the architecture a bit better - if such a feature would be
appreciate by others than just myself.

FWIW, René also cross-posted basically the same question to:
http://lists.qt-project.org/pipermail/interest/2017-October/028275.html

(Just in case someone is reading only either mailing list...)

Regards,
Kevin

Post by René J.V. Bertin
Thanks,
René

[René J.V. Bertin]

Post by Kevin Funk
Makes sense. Patches welcome.
Should be easy enough, you need to reparent the widget contained in the
QDockWidget, set the Qt::Window flag and show it again. Work from there.

Hmm, I can try this too. For now I've tinkered with changing the window flags back to Qt::Window when the floating state changes. That also works but it's tricky (because apparently platform-dependent). But reparenting could have more side-effects, have you tried re-docking the window for instance?

This is something that can probably much better be done upstream: a QDockWidget::setFloatAsRegularWindow(bool) could modify how QDockWidget::setFloating(true) "tears off" windows.

Normally I'm all for giving the user choice, but wouldn't it be sufficient (better) in this case to let the toolview decide rather than adding a menu to obtain a different kind of floating window? That would give 2 menu actions to detach the window, which comes with having to handle transitions between those modes and will probably also lead to surprises when the floating windows are restored as Qt::Tool instead of Qt::Window.

FWIW, automatic restore is tricky to get right with my approach too. There is no state change notification of course, and apparently QDockWidget::isFloating() cannot yet be queried reliably in the ctor. I've been trying to work around this by re-attaching the windows on exit using QCoreApplication::aboutToQuit, but that only works on Mac. On Linux I simply don't get the signal. Another thing that makes restore tricky is the fact that the (documentation) toolview is restored multiple times; when it's being restored in detached mode I can see the window open and close multiple time while the session loads (possibly once per project).

Post by Kevin Funk
http://lists.qt-project.org/pipermail/interest/2017-October/028275.html

That was a general how-to.

[Kevin Funk]

Post by René J.V. Bertin

Post by Kevin Funk
Makes sense. Patches welcome.
Should be easy enough, you need to reparent the widget contained in the
QDockWidget, set the Qt::Window flag and show it again. Work from there.

Hmm, I can try this too. For now I've tinkered with changing the window
flags back to Qt::Window when the floating state changes. That also works
but it's tricky (because apparently platform-dependent). But reparenting
could have more side-effects, have you tried re-docking the window for
instance?

No, as I expected you to try this out if you'd like to have the feature.

Post by René J.V. Bertin
This is something that can probably much better be done upstream: a
QDockWidget::setFloatAsRegularWindow(bool) could modify how
QDockWidget::setFloating(true) "tears off" windows.
Normally I'm all for giving the user choice, but wouldn't it be sufficient
(better) in this case to let the toolview decide rather than adding a menu
to obtain a different kind of floating window? That would give 2 menu
actions to detach the window, which comes with having to handle transitions
between those modes and will probably also lead to surprises when the
floating windows are restored as Qt::Tool instead of Qt::Window.

Agreed. Could be an option in the application's setting.

Post by René J.V. Bertin
FWIW, automatic restore is tricky to get right with my approach too. There
is no state change notification of course, and apparently
QDockWidget::isFloating() cannot yet be queried reliably in the ctor. I've
been trying to work around this by re-attaching the windows on exit using
QCoreApplication::aboutToQuit, but that only works on Mac. On Linux I
simply don't get the signal.

State change about what? Which signal do you not get? aboutToQuit()? Did
KDevelop crash?

Please don't start adding work-arounds (we won't accept them upstream) before
having understood the actual problem.

Post by René J.V. Bertin
Another thing that makes restore tricky is the
fact that the (documentation) toolview is restored multiple times; when
it's being restored in detached mode I can see the window open and close
multiple time while the session loads (possibly once per project).

Then please fix/investigate this first(?)

Post by René J.V. Bertin

Post by Kevin Funk
http://lists.qt-project.org/pipermail/interest/2017-October/028275.html

That was a general how-to.

https://stackoverflow.com/questions/46699115/qdockwidgets-how-to-get-regular-windows-in-detached-floating-mode

^ That, too? Same question, 1:1 copy from mail on the Qt interest list, fired
off almost at the same time.

Please have a read:
https://en.opensuse.org/openSUSE:Mailing_list_netiquette#Cross_posting


I'd recommend you to spend more time digging through code, studying it, trying
things out, polishing things, adding unit tests, *then* come up with a review
request and trigger discussions -- instead of bombarding everyone with mails
asking how to implement this and that. (It's not just us (KDevelop), and I
just see the fraction of your mails on mailing lists I'm tracking... (Qt,
KDE))

I'm sorry this may come off a bit harsh, but it's not like there's no
precedent here.

PS: And I just received another mail of you, 40 minutes after the last one,
posting yet another incomplete patch. That doesn't help anyone.


Regards,
Kevin

[René J.V. Bertin]

Post by Kevin Funk
No, as I expected you to try this out if you'd like to have the feature.

You did say that it seemed to do the job, but on closer inspection it turned out the snippet you posted wouldn't even build :)

Post by Kevin Funk

Post by René J.V. Bertin
Normally I'm all for giving the user choice, but wouldn't it be sufficient
(better) in this case to let the toolview decide rather than adding a menu
to obtain a different kind of floating window? That would give 2 menu

Agreed. Could be an option in the application's setting.

Are there other toolviews where standalone windows make sense, beyond the fact that floating windows do not in general work well when you're using a focus-follows-mouse mode under X11?

Post by Kevin Funk

Post by René J.V. Bertin
FWIW, automatic restore is tricky to get right with my approach too. There
is no state change notification of course, and apparently
QDockWidget::isFloating() cannot yet be queried reliably in the ctor. I've
been trying to work around this by re-attaching the windows on exit using
QCoreApplication::aboutToQuit, but that only works on Mac. On Linux I
simply don't get the signal.

State change about what?

The topLevelChanged signal signalling a change in the floating state.

Post by Kevin Funk
Which signal do you not get? aboutToQuit()?

Indeed. Not on Linux at least, but on Mac I've also seen cases in another ctor.

Post by Kevin Funk
Did KDevelop crash?

Not in relation to the above.

It's easy to make it crash after reparenting the widget away though: the m_area and m_view members become NULL pointers because of that.

Post by Kevin Funk
Please don't start adding work-arounds (we won't accept them upstream) before
having understood the actual problem.

I think I meant workaround in the sense of a way to avoid having to handle complexity here. Basically, revert the "unfloating" and dock the window when about to exit, if it was made into a separate window.

Post by Kevin Funk
Then please fix/investigate this first(?)

That issue has long proven to be beyond me.

Post by Kevin Funk
^ That, too? Same question, 1:1 copy from mail on the Qt interest list, fired
off almost at the same time.
https://en.opensuse.org/openSUSE:Mailing_list_netiquette#Cross_posting

Time for me to come off as a bit harsh, but I've been doing this a bit longer than you. I avoid cross-posting to MLs and newsgroups where I'm certain a majority of users will get duplicates. Qt's interest ML and StackOverflow don't fall under that category for me, and in my experience I get answers on either one or the other. I prefer to take my chances to getting answers by asking questions rather than by not asking them and trying to reinvent each wheel I need myself again and again. That's all the more true when dealing with collaborative software.


R.

[René J.V. Bertin]

Anyway, anyone else who'd like to see this and who'd also like to work on this?

R

[Sven Brauch]

Post by René J.V. Bertin
Time for me to come off as a bit harsh, but I've been doing this a
bit longer than you. I avoid cross-posting to MLs and newsgroups
where I'm certain a majority of users will get duplicates. Qt's
interest ML and StackOverflow don't fall under that category for me,
and in my experience I get answers on either one or the other. I
prefer to take my chances to getting answers by asking questions
rather than by not asking them and trying to reinvent each wheel I
need myself again and again. That's all the more true when dealing
with collaborative software.

But -- sorry to say -- you end up costing a lot of people a lot of time
for reading through all the text you write, without really creating any
value. I know the feeling of thinking "what the hell is happening here?
I need to ask someone who knows what's up" all too well, but the truth
is, usually nobody knows. Everbody needs to debug it first. Thus you can
either investigate yourself, or write an email describing your problem
in three pages of text and hope for somebody else to do exactly that
work for you. Which effectively doesn't help at all.

To be completely blunt, I think this kind of general technical
discussion is only efficient if a) we're talking about a really really
complicated, annoying issue which needs a combined team effort to solve,
or b) somebody on the list has recently worked on exactly that thing or
is known to have in-depth knowledge of the code so the question can be
answered very quickly (e.g. David Nolden when it comes to the itemrepo
stuff). Most of the other threads, to phrase it in a mean way, boil down
to "can somebody else please debug this for me".

Best,
Sven

[René J.V. Bertin]

Post by Kevin Funk
+ if (auto w = widget()) {
+ // turn into top-level window
+ w->setParent(nullptr);
+ w->setWindowFlag(Qt::Window, true);
+ w->show();

You'd want to add a close() after the w->show() to hide the now empty IdealDockWidget instance. And it (the instance) will need to save w, otherwise you'll get a crash when trying to re-attach the view:

@@ -140,6 +172,23 @@ void IdealDockWidget::contextMenuRequested(const QPoint &point)
setShortcut->setToolTip(i18n("Use this shortcut to trigger visibility of the toolview."));

menu.addSeparator();
+
+ QAction *separateWindow = nullptr;
+ if (isFloating()) {
+ separateWindow = menu.addAction(i18n("Turn into separate window"));
+ connect(separateWindow, &QAction::triggered, this, [this]() {
+ if (auto w = widget()) {
+ // turn into top-level window
+ qCWarning(SUBLIME) << "reparenting" << this << "->" << w << "away from" << w->parent();
+ m_windowWidget = w;
+ w->setParent(nullptr);
+ w->setWindowFlags(Qt::Window);
+ w->show();
+ close();
+ } } );
+ }
+
+ menu.addSeparator();
QAction* remove = menu.addAction(QIcon::fromTheme(QStringLiteral("dialog-close")), i18n("Remove Toolview"));

QAction* triggered = menu.exec(senderWidget->mapToGlobal(point));
@@ -176,11 +225,17 @@ void IdealDockWidget::contextMenuRequested(const QPoint &point)

return;
} else if ( triggered == detach ) {
- setFloating(true);
- m_area->raiseToolView(m_view);
+ if (!m_windowWidget) {
+ setFloating(true);
+ m_area->raiseToolView(m_view);
+ }
return;
}

+ if (m_windowWidget && triggered != separateWindow) {
+ setWidget(m_windowWidget);
+ }
+
if (isFloating()) {
setFloating(false);
}
@@ -197,9 +252,11 @@ void IdealDockWidget::contextMenuRequested(const QPoint &point)

Area *area = m_area;
View *view = m_view;
- /* This call will delete *this, so we no longer
- can access member variables. */
- m_area->moveToolView(m_view, pos);
- area->raiseToolView(view);
+ if (m_area) {
+ /* This call will delete *this, so we no longer
+ can access member variables. */
+ m_area->moveToolView(m_view, pos);
+ area->raiseToolView(view);
+ }
}
}

[René J.V. Bertin]

On Thursday October 12 2017 15:43:16 Kevin Funk wrote:

Hi,

Post by Kevin Funk
Seems to do the job. But please investigate some more yourself, please make
sure that doesn't break any other integration within the Sublime architecture.
Only then file a patch,

Almost there...

Post by Kevin Funk
best with some unit test.

I think the test_viewactivation test could be adapted to use IdealDockWidgets instead of QDockWidgets, but that requires exporting at least IdealDockController, IdealButtonBarWidget, IdealToolWidget and IdealLayout (I've already exported IdealDockWidgets itself so that doc plugins can configure it to use regular instead of floating windows).

I guess that those classes would require exporting for any unittest, is that worth it? I mean, you cannot really unit-test how those windows behave, if they render their content as it should etc, can you?

FWIW, I also get the test to link when I add the source files for the classes above to the test sources. The result doesn't crash straight away. I know this is safe with C, but is it also safe with C++?

R.

[Kevin Funk]

Post by René J.V. Bertin
Hi,

Post by Kevin Funk
Seems to do the job. But please investigate some more yourself, please make
sure that doesn't break any other integration within the Sublime
architecture. Only then file a patch,

Almost there...

Post by Kevin Funk
best with some unit test.

I think the test_viewactivation test could be adapted to use
IdealDockWidgets instead of QDockWidgets, but that requires exporting at
least IdealDockController, IdealButtonBarWidget, IdealToolWidget and
IdealLayout (I've already exported IdealDockWidgets itself so that doc
plugins can configure it to use regular instead of floating windows).
I guess that those classes would require exporting for any unittest, is that
worth it? I mean, you cannot really unit-test how those windows behave, if
they render their content as it should etc, can you?

That probably makes sense, yes.

Note that we shouldn't export the symbols of those classes in all cases (as it
has an performance impact).

Please try to use something like this (just hacked this together) for classes
only needed to be exported for unit tests:

diff --git a/kdevplatform/config-kdevplatform.h.cmake b/kdevplatform/config-
kdevplatform.h.cmake
index 7e3bdd9e0a..4ae2c01dd6 100644
--- a/kdevplatform/config-kdevplatform.h.cmake
+++ b/kdevplatform/config-kdevplatform.h.cmake
@@ -3,4 +3,12 @@

#define KDEV_ITEMREPOSITORY_VERSION @KDEV_ITEMREPOSITORY_VERSION@

+#cmakedefine01 BUILD_TESTING
+
+#if BUILD_TESTING
+#define KDEV_AUTOTEST_EXPORT(EXPORT_DEFINE) EXPORT_DEFINE
+#else
+#define KDEV_AUTOTEST_EXPORT(EXPORT_DEFINE)
+#endif
+
#endif
diff --git a/kdevplatform/sublime/idealbuttonbarwidget.h b/kdevplatform/
sublime/idealbuttonbarwidget.h
index f087f8ce7c..88289e4e71 100644
--- a/kdevplatform/sublime/idealbuttonbarwidget.h
+++ b/kdevplatform/sublime/idealbuttonbarwidget.h
@@ -23,6 +23,9 @@
#ifndef IDEALBUTTONBARWIDGET_H
#define IDEALBUTTONBARWIDGET_H

+#include "config-kdevplatform.h"
+#include "sublimeexport.h"
+
#include <QWidget>

class IdealToolButton;
@@ -40,7 +43,7 @@ class IdealDockWidget;
class View;
class Area;

-class IdealButtonBarWidget: public QWidget
+class KDEV_AUTOTEST_EXPORT(KDEVPLATFORMSUBLIME_EXPORT) IdealButtonBarWidget:
public QWidget
{
Q_OBJECT

Post by René J.V. Bertin
FWIW, I also get the test to link when I add the source files for the
classes above to the test sources. The result doesn't crash straight away.

Yep, but that a) increases compile, b) you need to collectively add all the
source files a certain source file depends on, too (maintenance burden)

Better use something like above.

Post by René J.V. Bertin
I know this is safe with C, but is it also safe with C++?

I think that hasn't anything to do with whether it's C or C++

Post by René J.V. Bertin
R.

Regards,
Kevin

[René J.V. Bertin]

On Saturday October 14 2017 11:34:31 Kevin Funk wrote:

I see I was just too quick with putting this on Phab...

Post by Kevin Funk

Post by René J.V. Bertin
I guess that those classes would require exporting for any unittest, is that
worth it? I mean, you cannot really unit-test how those windows behave, if
they render their content as it should etc, can you?

That probably makes sense, yes.

Sorry, which of my 2 questions makes sense?

Post by Kevin Funk
Note that we shouldn't export the symbols of those classes in all cases (as it
has an performance impact).
Please try to use something like this (just hacked this together) for classes

I've been able to restrict it to only IdealController, so the test builds without need for pulling in additional files. Is that an acceptable cost or should I use your trick for this class?

Your suggestion is to export only when BUILD_TESTING is true? If that's used elsewhere we'd have to document that the option does more than increase the build time. The performance impact aside it also means you can end up with visibility issues in builds with BUILD_TESTING=OFF (so the CI would need to verify both builds?).

Post by Kevin Funk

Post by René J.V. Bertin
FWIW, I also get the test to link when I add the source files for the
classes above to the test sources. The result doesn't crash straight away.

Yep, but that a) increases compile, b) you need to collectively add all the
source files a certain source file depends on, too (maintenance burden)

I noticed ... several times until I finally had added all required files.

Post by Kevin Funk

Post by René J.V. Bertin
I know this is safe with C, but is it also safe with C++?

I think that hasn't anything to do with whether it's C or C++

Doesn't (modern) C++ have class identities, type info and all kinds of similar metadata which could be different for the same code compiled in different modules?

R.