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

Itembox Implementation #63

Closed
wants to merge 10 commits into from
Closed

Itembox Implementation #63

wants to merge 10 commits into from

Conversation

FastestMolasses
Copy link
Member

Description

Adds the ItemBox component, which is used for the auto complete feature.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

@FastestMolasses FastestMolasses marked this pull request as ready for review December 22, 2024 11:44
Copy link
Contributor

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

I agree with @matthijseikelenboom this should be in the CESE repo.

We discussed briefly what it should be called, and I think you're right that it could be used for other things in the future but calling it CompletionWindow would be fine in that package. It can be extended later but we can avoid that complexity for now.

Also if you move it, you should be able to add the keybind you have in CE right now to the existing keybind handler in CESE. That might fix the issue you mentioned with not being able to close tabs. You'd then also have access to the theme colors for styling this window.

//

import AppKit
import LanguageServerProtocol
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using the CompletionItem type from the LSP library, this should define a protocol for completion items.

}

/// Padding at top and bottom of the window
let WINDOW_PADDING: CGFloat = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a private variable, or a static variable on the window controller.

}
}

class NoSlotScroller: NSScroller {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to a separate file.

}
}

public protocol ItemBoxDelegate: AnyObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be in it's own file

Copy link
Member

@tom-ludwig tom-ludwig left a comment

Choose a reason for hiding this comment

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

Amazing! I was wondering if it is necessary to add this functionality here or rather in CESE, what do you think?

Comment on lines +100 to +105
NotificationCenter.default.addObserver(
self,
selector: #selector(parentWindowDidResignKey),
name: NSWindow.didResignKeyNotification,
object: parentWindow
)
Copy link
Member

Choose a reason for hiding this comment

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

Not removing the observer could lead to retain cycles or redundant notifications.

}

/// Padding at top and bottom of the window
let WINDOW_PADDING: CGFloat = 5
Copy link
Member

Choose a reason for hiding this comment

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

We're a bit inconsistent with this. Here you declared the constant with capital letters and in the extension you have constants declared normally, within the class and in lowercase.

Copy link

@matthijseikelenboom matthijseikelenboom left a comment

Choose a reason for hiding this comment

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

Like I said In Discord, I'd suggest changing the name "ItemBox" to something more clearer. Other than the comments left by the other guys, I think it looks good

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

Successfully merging this pull request may close these issues.

4 participants