feat: add mouse speed control setting#8075
feat: add mouse speed control setting#8075onyedikachi-david wants to merge 1 commit intodeskflow:masterfrom
Conversation
sithlord48
left a comment
There was a problem hiding this comment.
Some small changes to the code are needed
Additionally :Idk how i feel about now adding the appconfig to the server. I have to look but for the other client only options i think we copy into the server config. Honestly it maybe time to for proper client config for per client config.
nbolton
left a comment
There was a problem hiding this comment.
Thanks for the PR. Please move the new setting to the 'Advanced' tab.
|
You can not land merges in our history, You must use the rebase option. (tried to disable this but we can only control the option when pr is made from this repo) Please update the pr to have a linear history. |
Alright. |
e6f5378 to
c4c3e0f
Compare
|
Done that, and addressed requested changes. |
|
Really sorry about that we do not want the update button to even show but we seam unable to remove it , let alone get it to be the default (i made a request to github about this). Just be sure to pick in the future to use the "Update With rebase" option if you use the web interface other wise be sure you on current master then git rebase master locally |
I actually read that in the wiki, just forgot. |
|
Its weird because if i make a Pr coming from deskflow repo the option works but if you make it from a fork it does not. Anyway thanks for understanding. |
|
Please do not correct the commit with another commit. |
|
Let's keep the commit messages concise too. |
|
@onyedikachi-david There is some linting issues as well as needing to be re based , you should include a new argument to the client / server to pass teh value instead or reading the appConfig. (see the Peer ID checking PR i have to see an example of this). Lmk if you like for me to take this over and get it finished up Should provide some insite on where to add the option. |
I'll try to finish it up before the end of the day. (It's past 7pm here) |
732d4d0 to
ee2f778
Compare
ee2f778 to
0a02682
Compare
0a02682 to
da36693
Compare
|
Please do not use Merge commits in the PR. |
adb27eb to
608cbfa
Compare
|
@sithlord48 I just addressed the requested changes |
sithlord48
left a comment
There was a problem hiding this comment.
Getting there. please use the resolve conversation button when you "fix" a review issue this will make it easier for us to track the changes.
This Windows test are testing my patience 😂 |
9532d37 to
b76aca2
Compare
|
Just wish I had a windows laptop to test this before pushing. I apologise for the hassle. @sithlord48 Please rerun. thanks |
|
it's an integration test that is failing, and i didn't write any. ./build/bin/integtests
result=$?
if [ $result -ne 0 ]; then
echo "Integration tests failed with code: $result" >> $GITHUB_STEP_SUMMARY
fi |
|
you may have changed something for one of the tests @nbolton ? |
I'll take a peek Edit: Error from CI: I managed to repro on my Windows dev machine, which shows more detail: I'll figure out a fix for you.
IDK how it passed before but it's late here and I'm too tired to figure out why. It'll no doubt come to me in a dream. Edit 3: On second thoughts, I found the reason why it's now happening and will highlight it in the review. |
b76aca2 to
d888915
Compare
@onyedikachi-david How about starting a VM and installing Windows? What OS are you on? Microsoft allows Windows ISO download for free: |
5174517 to
85884de
Compare
It was a unit test that was failing (not an integration test) and it was failing because you added an extra unnecessary call to |
There was a problem hiding this comment.
It's pretty clear this PR has not been tested by the contributor, which is disappointing.
There are also many clumsy and irrelevant changes.
@onyedikachi-david Final warning: I have set this PR to draft. When this PR is next set to open (ready for review), if it is not tested properly, or the next code review is as bad as this one, I will immediately close this PR. We cannot afford the time to babysit bounty PRs.
Edit: I have set this to draft. You can push to your branch but do not reopen this PR until you are 100% certain it is tested, working well, and high quality. Never open a PR with code you haven't tested.
| <item> | ||
| <widget class="QLabel" name="m_pLabelMouseSpeed"> | ||
| <property name="text"> | ||
| <string>Mouse Speed:</string> |
There was a problem hiding this comment.
I think what you had originally makes more sense.
| <string>Mouse Speed:</string> | |
| <string>Mouse speed multiplier:</string> |
src/lib/deskflow/ServerApp.cpp
Outdated
| // parse command line arguments | ||
| parseArgs(argc, const_cast<const char *const *>(argv)); | ||
|
|
There was a problem hiding this comment.
This is likely why the integ test was failing.
[ RUN ] ServerAppTests.runInner_will_handle_configuration_lifetime
[2025-01-29T21:44:03] ERROR: the --daemon argument is not supported on windows. instead, install deskflow-server as a service (--service install)
C:\Projects\deskflow\src\lib\deskflow\ArgParser.cpp:411
Command exited with code 3: ./build/bin/unittests
Why are you calling this here? Please remove it.
| // parse command line arguments | |
| parseArgs(argc, const_cast<const char *const *>(argv)); |
This is why you need to be able to run the tests locally on a Windows dev machine before setting the PR to open and asking for a review.
src/lib/gui/core/CoreProcess.cpp
Outdated
| args << "--tls-cert" << m_appConfig.tlsCertPath(); | ||
| } | ||
|
|
||
| // Add mouse speed argument if not default value |
There was a problem hiding this comment.
Please do not add comments explaining how the code works. Only add comments when you need to explain why you're doing something, which should be somewhat rare.
| // Add mouse speed argument if not default value |
src/lib/server/Config.cpp
Outdated
| m_events(events), | ||
| m_mouseSpeed(1.0) |
There was a problem hiding this comment.
Unnecessary, already using default init.
| m_events(events), | |
| m_mouseSpeed(1.0) | |
| m_events(events) |
src/lib/server/Config.h
Outdated
| bool m_hasLockToScreenAction; | ||
| IEventQueue *m_events; | ||
| std::string m_ClientAddress; | ||
| double m_mouseSpeed = 1.0; // Default mouse speed |
There was a problem hiding this comment.
Redundant comment.
| double m_mouseSpeed = 1.0; // Default mouse speed | |
| double m_mouseSpeed = 1.0; |
src/lib/server/Server.cpp
Outdated
| LOG((CLOG_DEBUG1 "DESKFLOW_MOUSE_ADJUSTMENT not set, using original values %+d,%+d", dx, dy)); | ||
| } | ||
|
|
||
| // adjust mouse speed using server config |
There was a problem hiding this comment.
| // adjust mouse speed using server config |
There was a problem hiding this comment.
None of the changes in this file should be necessary, please revert.
| EXPECT_FALSE(app.args().m_config); | ||
| // Initially config should be set but with test values | ||
| EXPECT_TRUE(app.args().m_config); | ||
| EXPECT_NEAR(1.0, app.args().m_config->mouseSpeed(), 1e-10); | ||
|
|
||
| const char *argv[]{"deskflow-server"}; | ||
| app.runInner(1, const_cast<char **>(argv), nullptr, [](int, char **) { return 0; }); | ||
|
|
||
| // After runInner, config should still be properly initialized | ||
| EXPECT_TRUE(app.args().m_config); | ||
| EXPECT_NEAR(1.0, app.args().m_config->mouseSpeed(), 1e-10); |
There was a problem hiding this comment.
I cannot see how any of this change is necessary or useful. Please revert this.
This comment was marked as duplicate.
This comment was marked as duplicate.
85884de to
eb05f93
Compare
eb05f93 to
0e9b1d1
Compare
|
rebasing and running . |
0e9b1d1 to
3e15c42
Compare
|
I'm sorry for keeping this on draft this long, although all the CI passed before the current conflict, I left it in draft because I wanted to demo it locally on a windows OS, had some issues with it (dependencies issues) while building. I tried alternative ways to do that but, the windows laptop isn't always with me, being a friends own; I have an M1 air, 256gb, with more than 85% disk usage, so can't run a VM on it as suggested by @nbolton. Hence I can not demo a working implementation on windows OS. Last week i tried resolving the conflicts, noticed that alot of renaming was done, making it difficult to resolve them, each resolve results to another conflicts. I am suggesting to open another PR for a cleaner merge. Is that okay with you @sithlord48 . |
|
I would prefer to keep the history all in this one place if you can |
3e15c42 to
0bd966d
Compare
0ddec7a to
0bd966d
Compare
|
Unfortunately, as this PR is taking too much of our time, I must close it. I am also removing the bounty from the issue. Thanks for your effort. |
|
@nbolton. Kinda sad though, but it was quite a learning experience nevertheless, I use my contributions to open source repo to both improve on my skills and up my hiring chance, thanks for the review and all. I'll do better when chanced to contribute again. |
I'd be glad to see a new PR once you have a bit more C++ experience. Good luck with your learning journey! |
Description
Added a new mouse speed control feature that allows users to adjust cursor movement speed when in client mode. This enhancement provides better control and customization of mouse behavior.
Changes
Testing
Screenshots
Notes
Related Issues
/claim #7582
Fixes: #7582