Add ability to Pin PopupDialog

[mickaelistria]

In some circumstances (eg comparisons), users may want PopopDialogs to remain visible when switching attention to another area. This introduces a "Pin dialog" menu (and a "Close") to allow the dialog to stick when users selects it.

Fixes #1905

[github-actions]

Test Results

 1 210 files  ±0   1 210 suites  ±0   1h 12m 36s ⏱️ + 19m 10s
 7 659 tests ±0   7 428 ✅  -  3  231 💤 + 3  0 ❌ ±0 
16 092 runs  ±0  15 579 ✅  - 44  513 💤 +44  0 ❌ ±0 

Results for commit 6656068. ± Comparison against base commit bc6e3ec.

This pull request skips 3 tests.

UiTestSuite org.eclipse.ui.tests.api.ApiTestSuite org.eclipse.ui.tests.api.WorkbenchPluginTest ‑ testGetImageRegistryFromAdditionalDisplay
org.eclipse.jface.text.tests.contentassist.ContextInformationTest ‑ testContextInfo_hide_focusOut
org.eclipse.urischeme.internal.registration.TestUnitWinRegistry ‑ testWinRegistry

♻️ This comment has been updated with latest results.

[jukzi]

The user can not know which Window belongs to which context

[mickaelistria]

The user can not know which Window belongs to which context

Indeed, and it would be nice to improve it some time. But still this improvement is helpful, doesn't harm standard usage nor the code.
So while things can -as always- be improved, for example by adding titles to pop-ups or more content in them, this cannot be a standard implementation in JFace anyway.
So I don't think your feedback can be fairly treated as a blocker. @eclipse-platform/eclipse-platform-project-leads Can some PLs please discuss here?

[iloveeclipse]

Few issues with the current PR:

1. Major one It changes menu for every popup dialog, even for those who probably never intended to be "pinned". I'm not sure
2. On Linux it doesn't work as expected: once focus is lost, pinned dialog is closed (tried with the Quick Type Hierarchy).
3. The menu text is not added to properties file.
4. The checkbox "pinned" state seem to be persisted. Not sure this is intended, as "pinning" is most likely needed only for special occasions, not by default.
5. Since pinning doesn't work for me I can't test how it would behave with multiple dialogs.

So far the major change I would expect here: this feature should be optional and client code should decide if it should be turned on.

[mickaelistria]

Major one It changes menu for every popup dialog, even for those who probably never intended to be "pinned". I'm not sure

Indeed, but actually, it may be occasionally useful be able to pin any pop-up dialog. Think about Ctrl+3 when you don't remember what you want to type in an need to close then reopen, or when using JDT open symbols.
It's not a very common use-case, but it can always be a helpful one.

On Linux it doesn't work as expected: once focus is lost, pinned dialog is closed (tried with the Quick Type Hierarchy).

OK, that's strange as I tested it works. But I will retry it with Quick Type hierarchy to make sure.

The menu text is not added to properties file.

Missing git add, sorry. I'll fix it.

The checkbox "pinned" state seem to be persisted. Not sure this is intended, as "pinning" is most likely needed only for special occasions, not by default.

What do you mean by persisted? The status should only affect the current dialog and not be persisted from 1 dialog to the other. If it does, it's indeed a problem.

So far the major change I would expect here: this feature should be optional and client code should decide if it should be turned on.

I think it's really making things much more complex than they have to be whule reducing the potential benefits of this feature to all PopupDialogs.

[mickaelistria]

The "Quick Text Hierarchy" seems to be a very particular case, it's actually the HierarchyInformationControl which itself implements JDT's AbstractInformationControl . This control seems to not close the dialog when clicking out, but instead just makes it invisible and reopens it later if it wasn't really closed (eg closed with Esc or an element got selected).

Here is the stacktrace hiding the dialog instead of closing it (thus rendering "Pin" non-functional)

Thread [main] (Suspended (breakpoint at line 625 in AbstractInformationControl))	
	HierarchyInformationControl(AbstractInformationControl).setVisible(boolean) line: 625	
	InformationPresenter(AbstractInformationControlManager).hideInformationControl() line: 1212	
	InformationPresenter.hideInformationControl() line: 407	
	InformationPresenter$Closer.mouseDown(MouseEvent) line: 158	
	TypedListener.handleEvent(Event) line: 209	

[iloveeclipse]

The "Quick Text Hierarchy" seems to be a very particular case

But this is an example of custom code that doesn't need "pin/close" actions and where adding actions that don't do anything would be understood as a bug by users. That is yet another reason to make the new code optional.

[mickaelistria]

ok.

[mickaelistria]

I've pushed a new patch taking an extra constructor flag.

[mickaelistria]

see consumer: eclipse-platform/eclipse.platform#1403

[iloveeclipse]

PR needs a rebase before merge anyway.

[iloveeclipse]

Just quickly scanning through the code it seem OK. Couldn't test as the second PR can't be rebased on master.

[mickaelistria]

@iloveeclipse I'd like to merge this soon. Any issue left?

[jukzi]

I am against this CR! the only proposed usecase does not work and i see no benefit over just seeing the values in the debug shell (press ctrl+shift d twice):

[mickaelistria]

the only proposed usecase

I think this can have merits for many, if not all popup dialogs. As it could allow to eg easily compare Type hierarchies and so on. But it requires adoption from consumers to enable more use-cases. At least, this PR makes things possible.

does not work

As demoed in eclipse-platform/eclipse.platform#1403 , this works, at least in some cases; that may not be all of them, but at least cover the initial story that brought to this contribution (comparing values from 2 distinct executions).
Let's continue the discussion about the case that work/don't work in eclipse-platform/eclipse.platform#1403 .

i see no benefit over just seeing the values in the debug shell

That's because your example here only deals with "flat" objects which have a nice toString() implemented. Debugging often requires to inspect more complex objects for which just printing is not the most efficient.

[jukzi]

i see no benefit over just seeing the values in the debug shell

That's because your example here only deals with "flat" objects which have a nice toString() implemented. Debugging often requires to inspect more complex objects for which just printing is not the most efficient.

For objects without toString() you can use the Expressions view (ctrl+shit+i twice):

the popups are not any better.

[mickaelistria]

For objects without toString() you can use the Expressions view (ctrl+shit+i twice):

To be honest, I didn't know that it was possible to send an "evaluated" expression to this view. I always type code in so this gets re-evaluated on every context change.
So indeed, this can cover a part of this story.
Still...

the popups are not any better.

They are often more usable to me, because they allow side by side comparison of values as trees, which is crucial in many cases.

[mickaelistria]

@jukzi I think all your concerns about working or not and overall usability/usefulness were covered. Anything else?

[iloveeclipse]

Please fix action name, the rest looks OK

[jukzi]

I tested it again. Closing with key shortcut works now. The related text in the pop up is wrong.

crtl-shift-i does not move but close the pop up. And when having multiple pop ups i now have multiple such "help" texts, Which are not helpfull at all, as only the active popup can use that key. So i suggest on pinning the popup that text is removed.

The main difference to pinning a value to the expression view is that pop ups don't close when the debugee is terminated. Which leads to VMDisconnectedException when using a pinned popup:

Also there is nothing like "close-all" for popups so that it is hard to clean up the screen.
In summary i highly dislike the whole workflow of using that menu to pin ,move and close the popup. It's much easier to just configure a screenshot tool like greenshot and take screenhots of the popup with a hotkey. So i would never use the Pin functionality myself. On the other hand that workflow is so well hidden that no normal user would ever find or use it anyway, so that it won't be a harm and i am not strictly against it. But is there anybody beside you who actually want's this unhandy feature?

[mickaelistria]

crtl-shift-i does not move but close the pop up

I think it's more a topic for eclipse-platform/eclipse.platform#1403 , but I'll try to make it work as this can be useful.

Which leads to VMDisconnectedException when using a pinned popup:

IMO, this is acceptable. Auto-closing a pinned dialog would semantically be incorrect. Keeping the dialog open and having such message showing its content are outdated is consistent with expectation.

Also there is nothing like "close-all" for popups so that it is hard to clean up the screen.

I don't think it's a major issue, and it's for sure not a regression. It would moreover be difficult to know what "Close All" mean: class all inspect windows, close all inspect windows for the current launch, close all pop-up dialogs...

But is there anybody beside you who actually want's this unhandy feature?

I know that many people want a more efficient way to compare values from different stackframes/launches. I don't claim this is the perfect way, but I think it's a better way than everything we have so those people would be happy to use it.

that it won't be a harm

Indeed.

"i am not strictly against it"