Skip to content
This repository has been archived by the owner on Mar 26, 2024. It is now read-only.

Add status endpoint #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add status endpoint #133

wants to merge 1 commit into from

Conversation

SerVB
Copy link
Member

@SerVB SerVB commented Feb 15, 2022

This PR adds a new /status endpoint. The main purpose is to use it in Projector in Space for auto hibernation and health check, but it can be also used any way you want.

Paired PR: JetBrains/projector-server#114

@SerVB SerVB force-pushed the add-status-endpoint branch from fe7fcb0 to 1b08e11 Compare February 15, 2022 16:49
@codecov-commenter
Copy link

Codecov Report

Merging #133 (1b08e11) into master (0a68d1e) will decrease coverage by 0.15%.
The diff coverage is 32.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #133      +/-   ##
============================================
- Coverage     25.19%   25.03%   -0.16%     
  Complexity       63       63              
============================================
  Files           132      133       +1     
  Lines          3890     3918      +28     
  Branches        407      409       +2     
============================================
+ Hits            980      981       +1     
- Misses         2889     2916      +27     
  Partials         21       21              
Impacted Files Coverage Δ
...ojector/common/protocol/toClient/StatusEndpoint.kt 0.00% <0.00%> (ø)
...ns/projector/server/core/websocket/HttpWsServer.kt 0.00% <0.00%> (ø)
...ector/server/core/websocket/HttpWsServerBuilder.kt 0.00% <0.00%> (ø)
.../projector/common/protocol/toServer/ClientEvent.kt 35.61% <57.69%> (-1.01%) ⬇️
...ns/projector/common/protocol/handshake/Constant.kt 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a68d1e...1b08e11. Read the comment docs.

@SerVB SerVB marked this pull request as ready for review February 15, 2022 17:27
@SerVB SerVB requested a review from ARTI1208 February 15, 2022 17:27

@Serializable
data class ClientClipboardEvent(
val stringContent: String, // TODO: support more types
) : ClientEvent()
) : ClientNonUserEvent()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why clipboard is not a user event? It's intentionally initiated by user

Copy link
Member Author

Choose a reason for hiding this comment

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

I consider this one as a non-user because it happens when clipboard content on the client side is changed. Well, in Web Client, it's currently generated just before a user presses Ctrl+V, but it's a limitation of the browser that we can't easily listen to clipboard updates system-wide (I have some noted thoughts how we can support listening in the future; and there shouldn't be any problems when implementing listeners for native apps).

So this is low-level clipboard sync event, it's not user-generated.

I agree it's unclear.

Do you think adding a comment for ClientClipboardEvent like the following is enough here?

/** A change of the clipboard content of the client-side has happened. */

Copy link
Contributor

Choose a reason for hiding this comment

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

I think comment should mention that change happened because of 'low-level clipboard sync event'


@Serializable
data class ClientSetKeymapEvent(
val keymap: UserKeymap,
) : ClientEvent()
) : ClientUserEvent()
Copy link
Contributor

Choose a reason for hiding this comment

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

Inverse situation of clipboard: isn't keymap updated automatically? Or please specify what you understand saying user and non-user event

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, in the Web Client, indeed this event is sent only during the start of the connection. However, it can be implemented that a user can send this event manually. That's why I've selected to make it ClientUserEvent.

I agree that it's a bit unclear how to divide events. Initial though for this is to allow the server to understand if this action is counted as user activity or not. Maybe the naming user and non-user isn't good here. Maybe we can have a call and brainstorm it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to schedule a call on Tuesday or Wednesday. Also I plan to work about a half of Monday, so it's also an option

Copy link
Member Author

Choose a reason for hiding this comment

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

This is PR is not in priority so let me add this topic to the weekly meeting

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants