Added hover UI for front end by nangtrongvuon · Pull Request #228 · xi-editor/xi-mac

nangtrongvuon · 2018-07-18T00:08:48Z

This is the hover part of #221. This PR adds the hover UI to Xi to work with the LSP's hover feature.

In order to use hover, Option+Click on the desired symbol.
This PR can be tested using my dummy plugin, or with xi-editor's #753.

This commit adds support for the `show_hover` notification that comes from core.

Hover results now display in a popover placed at the baseline of the selected symbol.

EditView will now update its tracking area for mouseMoved, to be used with the hover event.

The height calculation will now take in account the fixed width of 500 for the hover view.

Clarify that 500 was chosen because it is XCode's width for quick help.

cmyr · 2018-07-18T16:40:49Z

Okay playing around this seems excellent! The only visual issue I see is that a second option+click while the hover view is visible causes a jerky animation.

betterclever · 2018-07-18T23:26:22Z

I was testing this out, noticed a new notable bugs/ unintended behaviour.

The View is rendered a bit-off sometimes.
If you open a Hover and then scroll, the hover view keeps anchored to the original position. The ideal behaviour should be that it should dismiss or keep anchored to the textual position (not highly preferred)

I am not sure if it is happening with only me or is a general problem. I am using a multi-monitor setup so not sure if that is causing any error (since I read you discussing some issue about it on IRC) though I noticed it with a single screen plugged in as well.

nangtrongvuon · 2018-07-19T02:25:40Z

@betterclever Heya, can you explain a little bit more on how the view is rendered a bit off sometimes? As in, does the arrow point to weird places?

Gotcha on the popover behaviour, I'll try to tweak it and find out what works best.

betterclever · 2018-07-19T06:11:00Z

It is a but hard to describe it here. Probably it is best to do a screenshare where I can show you what is happening in my computer.

cmyr

Some observations. There are also two rendering issues, one of which is a bug and one of which is maybe more of a design thing, but both of which are present in this screenshot:

The bug is that the vertical position of the hover view does not take the scroll position into account. (My cursor is on line 16)

The design thing is that I think the hover view should shrink horizontally (with a minimum size) if the text its displaying does not fill the view.

Otherwise this is looking good!

cmyr · 2018-07-20T15:58:01Z

+    var hoverRequestID = 0
+
    override func viewDidLoad() {
+        if let path = ProcessInfo.processInfo.environment["PATH"] {


we probably don't need to commit this.

cmyr · 2018-07-20T16:02:04Z

    }
-
+
+    override func mouseMoved(with theEvent: NSEvent) {


is this supposed to do anything? I can't seem to get it to work.

This is the one part that I don't think this implementation would do justice. In order to get us a real hover, some more work that hooks on text is required. For now, Option+Click is the most reliable option when trying to get a hover to work.

cmyr · 2018-07-20T16:02:57Z

+                let hoverPosition = editView.bufferPositionFromPoint(event.locationInWindow)
+                hoverTimer?.invalidate()
+                hoverTimer = nil
+                document.sendRpcAsync("request_hover", params: ["request_id": hoverRequestID, "position": ["type": "utf8_line_char", "line": hoverPosition.line, "character": hoverPosition.column]])


If we're going to use this in two places, I would add a method to Document that handles the implementation.

I believe that the RPC expected in core has changed as well; we don't send type and we send column instead of character.

cmyr · 2018-07-20T16:03:56Z

+        if gestureType == "request_hover" {
+            document.sendRpcAsync("request_hover", params: ["request_id": hoverRequestID, "position": ["type": "utf8_line_char", "line": position.line, "character": position.column]])
+            hoverEvent = theEvent
+            hoverRequestID += 1


we don't seem to actually ever use this anywhere. Perhaps it's unnecessary?

It is used for when an Option+Click is detected.

cmyr · 2018-07-20T16:04:41Z

+    // Lets AppKit do all the work when setting up a new text view
+    override init(frame frameRect: NSRect, textContainer container: NSTextContainer?) {
+        let textView = NSTextView(frame: .zero)
+        super.init(frame: frameRect, textContainer: textView.textContainer)


this override doesn't seem to actually do anything. What happens if we just leave it out?

init(frame, container) is the designated initializer for NSTextView subclasses. I had to let Cocoa make a text container on its own and use that container, which also contains conveniently created a NSLayoutManager and a NSTextStorage for my own NSTextView, since otherwise I would have had to create my own collection of objects associated with my custom text view.

cmyr · 2018-07-20T16:06:06Z

+    }()
+    var resultContent: String
+    var hoverView: HoverView
+    let hoverPopoverWidth: CGFloat = 500 // XCode size for quick help popovers


I think we should probably have a minimum and a maximum width, and then we resize ourself to the preferred width of our text, within those constraints.

I tried this, and there is a whole host of unintended behaviors when I tried to get the view to size itself to fit all the text perfectly.

Through much trial and error I and lots of reading I realized many macOS popovers (not just in XCode) works best when they have a fixed width or fixed height, because it's much easier to calculate how much space text would take in one direction. Fixed width was chosen because scrolling vertically is much more intuitive than scrolling horizontally.

Also, I think it looks a bit more aesthetically more pleasing this way. :)

Popovers now take in account the current scroll origin, which would fix popovers showing up at the wrong places. This commit also justifies the text which is in the hover's content.

betterclever · 2018-07-21T16:57:00Z

+    @objc func sendHover() {
+        if let event = hoverEvent {
+            let hoverPosition = editView.bufferPositionFromPoint(event.locationInWindow)
+            hoverTimer?.invalidate()


Compilation is failing for me on these two lines. Looking more closely, I see it is not declared anywhere.

Also, after removing these lines, hover does not work. Am I missing something?

No, this was my fault, I hastily committed these. It should be working now.

The hover needs more work before it can be implemented. The state that is in the PR right now is not sufficient.

This commit fixes an issue that can be see on white backgrounds, where the scroll view kept the popover's background, causing it to show up and making the popover corners not actually look rounded.

This will make it clearer that holding Option will enter an "inspection" state, and clicking on valid symbols will trigger a hover.

This commit removes background off the popover's content views, so it uses the natural popover background, maintaining consistency with other macOS apps.

cmyr

Okay, one more little thing and I think this is good to go.

cmyr · 2018-07-23T15:52:09Z

+        infoPopover.contentSize = hoverContentSize
+        infoPopover.contentViewController = hoverViewController
+
+        if let event = hoverEvent {


along with checking for hoverEvent, we should also make sure that the request_id we get back is the same is current. Otherwise we can have a situation where we request hover multiple times, and then the first result arrives and is displayed even though it isn't what we want.

The scroll view will now only expand in height up to the predefined popover width, to prevent cases where an overly long comment will cause a very tall popover to show.

cmyr · 2018-07-24T13:18:34Z

            let positioningSize = CGSize(width: 1, height: 1) // Generic size to center popover on cursor

            infoPopover.show(relativeTo: NSRect(origin: positioningPoint, size: positioningSize), of: self.view, preferredEdge: .minY)
+            hoverRequestID += 1


Doesn't this mean multiple requests could be sent with the same ID?

Yes, but I'm not quite sure what the request ID is meant to represent in the first place. As it is right now, the ID doesn't get incremented if a result wasn't shown, which happens when:

The result we get back is empty (as in, fallback errors and the like)

The request ID in the RPC notification we get from core is different from what's currently expected

Since (in my personal opinion) I think that hover requests should return results instantly, it's fine even if we send many requests with the same ID?

Now request IDs are checked to make sure the hover results shown are not stale.

Having the popover animate felt jarring when quickly switching between different symbols, and as such animations were disabled. This also gives the advantage and illusion of a smoother experience.

cmyr

Okay this is working well for me. Will hold the merge until we get the core work in, but lets freeze this and focus on auto-complete. 👍

betterclever · 2018-07-25T19:00:04Z

+            infoPopover.performClose(self)
+        }
+
+        let hoverContent = result["content"] as! String


As per latest work in xi-editor/xi-editor#753 , the result is now just a String, so you don't need to extract content out here.

show_hover has been changed to handle a String instead of a Dictionary.

cmyr

👍

nangtrongvuon added 6 commits July 17, 2018 22:39

Add support for core show_hover RPCs

3268dc4

This commit adds support for the `show_hover` notification that comes from core.

Make hover result display in popover

d570f63

Hover results now display in a popover placed at the baseline of the selected symbol.

Set edit view to update tracking area

74ef6cc

EditView will now update its tracking area for mouseMoved, to be used with the hover event.

Adjusted baseline calculation

86fe232

Corrected height calculation

79e5bfd

The height calculation will now take in account the fixed width of 500 for the hover view.

Rewrote some comments

b6e1500

Clarify that 500 was chosen because it is XCode's width for quick help.

Fix jerky animation when hovering the same symbol

93f44ec

nangtrongvuon force-pushed the hover-frontend branch from 75e52f1 to 205c24f Compare July 19, 2018 04:16

Close popover on scroll, changed popover behaviour

6492225

nangtrongvuon force-pushed the hover-frontend branch from 205c24f to 6492225 Compare July 19, 2018 04:16

cmyr reviewed Jul 20, 2018

View reviewed changes

Add scroll origin to hover, justify hover content

7d95324

Popovers now take in account the current scroll origin, which would fix popovers showing up at the wrong places. This commit also justifies the text which is in the hover's content.

nangtrongvuon force-pushed the hover-frontend branch 2 times, most recently from 9c1bf6c to 1107f7c Compare July 21, 2018 16:23

betterclever reviewed Jul 21, 2018

View reviewed changes

Prevent showing popover if result is empty

aa4c84f

nangtrongvuon force-pushed the hover-frontend branch from 1107f7c to a95dbb1 Compare July 22, 2018 00:22

Remove mouse moved hover implementation

45db8b3

The hover needs more work before it can be implemented. The state that is in the PR right now is not sufficient.

nangtrongvuon force-pushed the hover-frontend branch from a95dbb1 to 45db8b3 Compare July 23, 2018 01:39

nangtrongvuon added 3 commits July 23, 2018 08:59

Fixed corner not being completely rounded

3094141

This commit fixes an issue that can be see on white backgrounds, where the scroll view kept the popover's background, causing it to show up and making the popover corners not actually look rounded.

Turn cursor into crosshair when holding Option

e5c2aaf

This will make it clearer that holding Option will enter an "inspection" state, and clicking on valid symbols will trigger a hover.

Use popover natural background

a655ad7

This commit removes background off the popover's content views, so it uses the natural popover background, maintaining consistency with other macOS apps.

cmyr reviewed Jul 23, 2018

View reviewed changes

nangtrongvuon added 2 commits July 24, 2018 09:33

Check for requestID before showing hover

9e131b8

Add max height to scroll view

cddcd41

The scroll view will now only expand in height up to the predefined popover width, to prevent cases where an overly long comment will cause a very tall popover to show.

cmyr reviewed Jul 24, 2018

View reviewed changes

nangtrongvuon added 2 commits July 24, 2018 20:58

Adjust request ID management

ebf548a

Now request IDs are checked to make sure the hover results shown are not stale.

Disable animations for hover popover

6175111

Having the popover animate felt jarring when quickly switching between different symbols, and as such animations were disabled. This also gives the advantage and illusion of a smoother experience.

cmyr approved these changes Jul 25, 2018

View reviewed changes

betterclever reviewed Jul 25, 2018

View reviewed changes

Change hover RPC handling to match core

cb72cf4

show_hover has been changed to handle a String instead of a Dictionary.

cmyr approved these changes Jul 26, 2018

View reviewed changes

cmyr merged commit ff95951 into xi-editor:master Jul 26, 2018

Conversation

nangtrongvuon commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmyr commented Jul 18, 2018

Uh oh!

betterclever commented Jul 18, 2018

Uh oh!

nangtrongvuon commented Jul 19, 2018

Uh oh!

betterclever commented Jul 19, 2018

Uh oh!

cmyr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nangtrongvuon Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nangtrongvuon Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nangtrongvuon commented Jul 18, 2018 •

edited

Loading

cmyr left a comment •

edited

Loading

nangtrongvuon Jul 21, 2018 •

edited

Loading

nangtrongvuon Jul 24, 2018 •

edited

Loading