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.

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

Arguments tooltip is lost when certain topmost window steals focus #124

Open
sshukurov opened this issue Dec 6, 2022 · 17 comments
Open

Comments

@sshukurov
Copy link
Contributor

Hi @govert,

Our team has been struggling with an unpleasant issue when at times the arguments tooltip doesn't pop up when needed. We tracked it down and created a minimal repro:

EXCEL_7gK5ojzvFk

Steps to reproduce:

  1. Follow the "Getting started" guide in https://excel-dna.net/
  2. Add an ExcelArgument attribute to name parameter of SayHello method, e.g.: [ExcelArgument(Name = "Name", Description = "Enter your name")]
  3. Build and run the addin
  4. Load ExcelDna.IntelliSense[64].xll
  5. Focus on a cell and press Windows key and plus sign - Magnifier appears
  6. Close Magnifier (optional)
  7. Type =SayHello(

Expected: Arguments tooltip must show up.
Actual: It does not show up, closing the Magnifier doesn't help.

More investigation by our team:
We turned on diagnostic logging and saw that the problem roots in FormulaEditWatcher.cs. It appears that it normally expects events in the following order: Focus > Show > Unfocus > Hide. In our case however, we saw events in the produced log in Focus > Unfocus > Hide order.

We attempted to add some compensating code to IntelliSense and it helped:
image

If that works, I could submit a pull request. Otherwise, we would love to hear your advice on this matter.

@wh1t3cAt1k
Copy link

The sporadic missing tooltip window issue reproes quite stably for several members of our company (all with multi-screen setups), albeit not using the screen magnifier, but in their logs, the sequence of events is the same.

It's been occurring more frequently recently than before, which might be related to a recent Excel update.

@gmichaud
Copy link
Contributor

gmichaud commented Dec 6, 2022

On my side, the issue is especially obvious when doing a demo of our product on a Zoom/Teams call. Both products have some sort of top-most window/UI that may be interferring with focus the same way @sshukurov is able to with the Magnifier.

@govert
Copy link
Member

govert commented Dec 7, 2022

Hi @sshukurov - Yes, I can reproduce this too.

All of the state tracking for the windows is just derived from what I could see happening, so I'm not surprised there are scenarios where it goes wrong.

A pull request would be very welcome. Do you understand why your suggested change would fix the event scenario Focus -> Unfocus -> Hide, since we don't do currently do anything in Show or Hide anyway?
I can try to track it down with a bit more logging, but I'm sure you've noticed how tedious it is tracking and debugging this kind of UI interaction.

sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Dec 7, 2022
@sshukurov
Copy link
Contributor Author

@govert,

I know why it fixes, yes it was hard to debug and I was able to get some understanding only through studying the logs.

In FormulaEditWatcher when Unfocus triggers:

  • _formulaEditFocus becomes None
  • FormulaEditWindow would return IntPtr.Zero
  • CurrentPrefix is set to null

Consequently in IntelliSenseDisplay:

  • _argumentsToolTip doesn't get initialized
  • FormulaEditTextChange events are not processed

So in Show I'm simply instructing we regained focus if we thought it was lost, and vice versa in Hide. I might be wrong but feels like window focus and input focus might mean different things 🤔?

What I don't actually understand is why the need for Show and Hide handlers if they are currently effectively a noop? Was that intentional or were they added with a look to the future?

@sshukurov
Copy link
Contributor Author

#125

@sshukurov
Copy link
Contributor Author

@govert one of my colleages informed me today that the arguments tooltip gets hidden even with my proposed fix with a slight change to the above steps:

  1. Follow the "Getting started" guide in https://excel-dna.net/
  2. Add an ExcelArgument attribute to name parameter of SayHello method, e.g.: [ExcelArgument(Name = "Name", Description = "Enter your name")]
  3. Build and run the addin
  4. Load ExcelDna.IntelliSense[64].xll
  5. Focus on a cell and press Windows key and plus sign - Magnifier appears
  6. Type =SayHello(
  7. Press ALT + TAB to switch to another program
  8. Press ALT + TAB again to switch back to Excel

Magnifier must be kept open to be able to reproduce this one.

Surface investigation revealed that the tooltip gets hidden here because of Unfocus event, and strangely neither Focus nor Show does not fire upon ALT + TAB to switch back to Excel.

My first thought as a potential remedy was to change Unfocus handler to noop:
image

I'm slightly afraid, however, that this change might break existing functionality. I'm completely unaware for example, whether those events trigger in the same manner in different versions of OS and/or Excel 🤔

@wh1t3cAt1k
Copy link

@govert we're merging @sshukurov 's changeset into our fork of IntelliSense, including his last screenshot... Please let us know if you have any reservations or concerns about it

@govert
Copy link
Member

govert commented Dec 12, 2022

@wh1t3cAt1k No, I have no particular concerns with using only Focus and Hide as the event pair.
If you update the PR here I'm happy to merge in for the next version.

@sshukurov You said:

What I don't actually understand is why the need for Show and Hide handlers if they are currently effectively a noop? Was that intentional or were they added with a look to the future?

I don't recall the details, but I presume I added cases to allow logging of any event that might be useful, then tried to use the logging to figure out a good pair to do the tracking.

I have no insight about other Windows or Excel versions.

sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Dec 12, 2022
govert added a commit that referenced this issue Jan 18, 2023
…ltip-lost-when-topmost-window-steals-focus

#124 Fix case when arguments tooltip is not displayed when a topmost window steals focus
@sshukurov
Copy link
Contributor Author

@govert this issue had been fixed for us for a while, before it re-surfaced recently with exactly the same symptoms. Closer investigation revealed that the regression happened after the changes made per #123.

Our team troubleshooted this and we came to understanding that EVENT_OBJECT_LOCATIONCHANGE was necessary for the arguments tooltip to work properly but the event was replaced in the issue mentioned above.

We referred to the official docs for the event, where it is said to be triggered for caret and window objects:
image

That knowledge allowed us to assume we could re-enable the handling of it, only in the context of "caret" objects - to restore the proper behavior of the arguments tooltip, at the same time keeping the fix for #123.

We already made the relevant changes in our fork and got approval from our QA team. I'm going to proceed with a pull request to this repo and shall wait for you further feedback.

cc @sbolofsson

sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jun 14, 2023
…ng issue by re-enabling LOCATIONCHANGE event but only when fired for the caret
@sshukurov
Copy link
Contributor Author

#130

govert added a commit that referenced this issue Jun 19, 2023
…ltip-regression

#124 Troubleshoot IntelliSense arguments tooltip not appearing issue …
govert added a commit that referenced this issue Jun 19, 2023
…g issue by re-enabling LOCATIONCHANGE event but only when fired for the caret"

This reverts commit 62d1811.
@govert
Copy link
Member

govert commented Jun 19, 2023

Hi @sshukurov - I tried the pull request #130, but had to revert it again :-( With those changes I get back to the very laggy window movement of #123 (whether some IntelliSense pop-up is showing or not). I suspect that the moment we register to getting the extra WinEvent events, the native-> managed transitions are overwhelming (given the message processing changes that we think was made in some recent Excel version). I don't know how your private branch is performing - is your window dragging OK?
There might still be some way to register for some extra WinEvents without getting overwhelmed.
I'm not able to get in to this too deeply myself now, so I appreciate the contribution a lot.

@wh1t3cAt1k
Copy link

@govert as @sshukurov mentioned, we discovered that subscribing to location change at least for the caret was critical because otherwise the IntelliSense argument tooltip was sometimes not appearing while the user entered the formula in-cell.

At the same time, we can confirm that it returned the laggy behaviour.

We have three questions:

  1. Is there a way to subscribe to location change only for the caret? Apparently you're right about just the managed transition overwhelming the processing even though we're filtering by caret object down the road.

Because here there is no hwnd filter:

            _windowStateChangeHooks.Add(new WinEventHook(WinEventHook.WinEvent.EVENT_OBJECT_CREATE, WinEventHook.WinEvent.EVENT_OBJECT_LOCATIONCHANGE, syncContextAuto, syncContextMain, IntPtr.Zero));

Maybe we could subscribe to location change in a separate call and pass the hwnd of the "most granular thing" closest / most relevant to the caret? Any ideas how to obtain it?

  1. If it's managed transition that overwhelms the processing, can we do relevant filtering in an unmanaged component somehow outside of ExcelDNA & .NET (would you have advice on how to approach this).

  2. Instead of locationchange and caret object, would you think there are alternative events that one could try to stabilize the problem of arguments tooltip not appearing?

@wh1t3cAt1k
Copy link

Another thought @sshukurov @govert , if the issue could be related to the fact that the event handler runs on Excel's main thread (on the off chance it's not native/managed transition that is lagging).

Perhaps one thing that could be tried is to subscribe to EVENT_LOCATION_CHANGE (only this event) on a dedicated non-main thread and see if it helps?

sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
@sshukurov
Copy link
Contributor Author

Hi Govert!

I spent a while trying to implement what was proposed by @wh1t3cAt1k:

Perhaps one thing that could be tried is to subscribe to EVENT_LOCATION_CHANGE (only this event) on a dedicated non-main thread and see if it helps?

Unfortunately I wasn't able to arrive at a working solution - I was getting a successful hook registration but apparently SetWinEventHook hooks to UI events on the thread it is invoked from, and not the main UI thread - I don't have any other explanation.

So instead I decided to try a workaround where we would unhook from EVENT_LOCATION_CHANGE whenever we encounter an EVENT_SYSTEM_MOVESIZESTART, and hook again when we get an EVENT_SYSTEM_MOVESIZEEND.

Again, our QA team verified the workaround against this issue as well as #123, so I proceeded to submitting a PR for your review and testing:

#131

Thank you!

@sshukurov
Copy link
Contributor Author

Govert, please hold on reviewing, we are seeing some inconsistent behavior with my latest changes.

I'll get back here as soon as I have an update.

sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
sshukurov added a commit to VelixoSolutions/IntelliSense that referenced this issue Jul 6, 2023
@sshukurov
Copy link
Contributor Author

@govert I updated the PR - I moved the hooking/unhooking of EVENT_LOCATION_CHANGE to WindowLocationWatcher.

Our team carefully tested the changes this time - no window lagging and no issues with IntelliSense so far.

Please consider on your end.

@govert
Copy link
Member

govert commented Jul 19, 2023

@sshukurov Your changes should be in 1.7.0-RC6 and from what I can see it works well.
Thank you very much for the contribution.

I suppose (to answer the question from @wh1t3cAt1k before) that the ideal solution is to filter the WinEvents stream in native code, and have some mechanism to forward only the important information to the managed code. But the price of adding a native component to the IntelliSense library would be high - for example it would not be compatible with the Excel-DNA packing. One can go down the rabbit hole of have a native component managed from a resource, or making an executable region in data. But it doesn't seem like the right solution in this case.

I'm happy if we can get to a reliable plan that stays in the managed world, and I think the position-update-after-move behaviour is fine for this case.

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

No branches or pull requests

4 participants