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.

Sign up for GitHub

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

Jump to bottom

Add vncviewer option to use large cursor instead of dot #1491

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add vncviewer option to use large cursor instead of dot #1491

wants to merge 2 commits into from

Conversation

krystof1119
Copy link
Contributor

This PR adds a large cursor bitmap to both versions of vncviewer
(Java and native) which can be used instead of the previously used dot
cursor, as well as the options and parameters to be able to enable or
disable its use at runtime.

Resolves #1483

CendioOssman
CendioOssman requested changes Jul 6, 2022
View reviewed changes
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

I'm afraid this is pending more details on #1483. I don't want to add more clutter to the options without a good understanding of why that's the only way forward.

@krystof1119
Copy link
Contributor Author

Okay, that's fair. I'll leave this PR open for when we (hopefully) get more info from the requester.

@krystof1119 krystof1119 mentioned this pull request Jul 6, 2022
Merged
@CendioOssman
Copy link
Member

It doesn't seem like any real progress is being made on #1483, but the issue seems to be better support for VNC servers without fancy cursor handling.

With that in mind, we can probably look at progressing here. Are you still interested in working on this, @krystof1119?

@krystof1119
Copy link
Contributor Author

It depends on the scope of the project. If all we're talking about is adding an option to change the local cursor displayed, I could probably build on what I had made previously, but I don't have much experience with the VNC protocol itself, so if the changes were more involved than that, I might have to see whether or not it's beyond me.

@CendioOssman
Copy link
Member

No protocol changes, @krystof1119. Just better client-side options.

CendioOssman
CendioOssman requested changes May 20, 2024
View reviewed changes
vncviewer/Viewport.cxx Outdated Show resolved Hide resolved
vncviewer/parameters.cxx Outdated Show resolved Hide resolved
@krystof1119
Copy link
Contributor Author

Okay, I'll try and get a bit of work done on this - probably not going to have time this week, but I should be able to look at this after.

@krystof1119
Copy link
Contributor Author

It took me longer than I thought to get around to this, but I've got an implementation done in a different branch to the one in this PR (multipleCursorTypes). A few notes:

  • It seems that the frameworks don't allow getting the bitmap of the system mouse pointer, so I needed to special-case the calls to set the mouse pointer (the code is a bit ugly, I might still change it).
  • I renamed the config option to show the dot cursor since it's not necessarily a dot cursor anymore. I don't know if I should put in some code to handle setting the new config option if the old is detected, or if it would be better to keep the old name (even if it's not accurate anymore).
  • This is my first time working with these frameworks, so I'd appreciate it if someone went over my code, just to check I haven't done something stupid. Tying into this: I implemented the radio buttons with a string configuration option (it looks like that's how they're implemented in the rest of the codebase), so I need to run strcmp every time the cursor is set. This only seems to happen whenever the cursor enters the window area, so it's not too expensive, but it still feels weird.

I can force-push the changes onto the branch associated with this PR after someone reviews the code, I just didn't want to do it before.

@CendioOssman
Copy link
Member

It's difficult to review it if it's not here in the PR. Please do a force push and we can have a look at the details.

@krystof1119 krystof1119 force-pushed the largeCursor branch 2 times, most recently from a1d1356 to 612e5a5 Compare June 30, 2024 17:40
@krystof1119
Copy link
Contributor Author

Okay, I've force pushed my code to this branch. I've also made one minor change to make the CI checks pass (I had used a NULL somewhere a nullptr is now expected).

CendioOssman
CendioOssman requested changes Aug 6, 2024
View reviewed changes
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I've been away for a few week.s

Looks really promising! Just a few tweaks needed.

vncviewer/OptionsDialog.cxx Outdated Show resolved Hide resolved
vncviewer/parameters.cxx Show resolved Hide resolved
vncviewer/parameters.cxx Outdated Show resolved Hide resolved
vncviewer/parameters.cxx Outdated Show resolved Hide resolved
vncviewer/Viewport.cxx Outdated Show resolved Hide resolved
@krystof1119
Copy link
Contributor Author

I've made the requested changes, including changes to the equivalent parts of the Java version.

@krystof1119
Copy link
Contributor Author

The previous version did not work correctly with the system cursor enabled after the VNC client lost and regained focus, this fixes it (my bad for pushing without testing that).

CendioOssman
CendioOssman requested changes Aug 9, 2024
View reviewed changes
vncviewer/Viewport.cxx Outdated Show resolved Hide resolved
vncviewer/Viewport.cxx Outdated Show resolved Hide resolved
vncviewer/Viewport.cxx Outdated Show resolved Hide resolved
vncviewer/Viewport.cxx Show resolved Hide resolved
vncviewer/Viewport.h Outdated Show resolved Hide resolved
@krystof1119 krystof1119 force-pushed the largeCursor branch 2 times, most recently from 5b00c24 to f4e0425 Compare August 9, 2024 16:32
@krystof1119
Add local cursor selection for C++ version
c959c84 
This adds the option to select which cursor should be used in the event the
server sends an invisible cursor. It also renames the DotWhenNoCursor config
option to AlwaysCursor.
@krystof1119
Copy link
Contributor Author

Sorry about the multiple separate force pushes, I noticed some of your comments too late.

I was hoping to avoid calling strcasecmp every time the cursor changes, but I imagine it's not going to be noticeable performance-wise. Changing the cursor type while the Java VNC viewer is running is still not working entirely correctly, but I don't think that's solvable without more extensive changes (and it seems like it wasn't working perfectly before, so at least it's not a regression).

I also decided to change the fallback cursor type to "Dot" in the options dialog rather than changing it to "System" here, since that was the previous behavior.
Other than that, I've made the two remaining changes, thanks for catching that.

CendioOssman
CendioOssman approved these changes Aug 13, 2024
View reviewed changes
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work!

@bphinz, are you happy with the Java version?

bphinz
bphinz reviewed Aug 14, 2024
View reviewed changes
java/com/tigervnc/vncviewer/OptionsDialog.java Outdated
JLabel cursorTypeLabel = new JLabel("Cursor type:");
String[] cursorTypes = {"Dot", "System"};
cursorTypeChoice = new MyJComboBox(cursorTypes);
cursorTypeChoice.setEditable(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this editable? Doing so allows for an arbitrary string to entered but then subsequently only checks for "Dot" or "System".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I got setEditable confused with setEnabled and didn't notice the difference in behavior when testing. I've corrected it now.

@krystof1119
Add local cursor selection for Java version
7f44ab9 
Replicates the C++ work done in the previous commit on the Java version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bphinz bphinz bphinz left review comments

@CendioOssman CendioOssman CendioOssman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vncviewer - show mouse symbol rather than dot when no cursor . [$100]
3 participants
@krystof1119 @CendioOssman @bphinz