Pointer mode (Zoom) changes on double click (same behavior as of official software) #54

mayanksuman · 2019-11-17T19:51:43Z

The Zoom is toggled if user double tap the pointer (top most) button (within 300ms; can be changed as it is part of Setting).

A major redesign of events handling has occurred. As it was necessary that the double click (and associated two clicks) events should not be passed to presentation so projecteur now grabs the input from SpotLight device. However, all other events (except Mouse Click) is passed again to the system by using uinput interface. The current redesign can help in giving solution to #43 without using a custom bash script.

Double Click is not passed to system rather used for changing the mode of presenter (toggling Zoom Setting). This is same behaviour as official software.

Further Work: To introduce the third mode (Digital Laser) and additionally the provision for different options/setting for each of the mode.

…velop

jahnf · 2019-11-18T05:34:25Z

Thanks for the pull request. 👍
We can also reuse grabbing the device for key assignment later (later == please keep pull requests ~~small~~ focused on one feature/fix)

While the idea of predefined spotlight settings (you call it pointer modes) is good,
I don't necessarily want to do the same as the official software - why just copy everything?

Code Review follows probably tonight. (Central European Time)

mayanksuman · 2019-11-18T06:15:51Z

A clarification: predefined spotlight settings does not mean hardcoded. I want to provide user the option that they can define three different set of settings -- Highlight, SpotLight(Zoom) and Digital Laser. The mode can customized by user.

No, We are not copying everything. The idea of customized key event on Next/Back press is unique to Projecteur. However, I do not want that the users of Projecteur feel limited when compared to their experience over Win/Mac.

In a way, Projecteur will provide unique feature that is not present in official software.

jahnf

Hey, so I had a quick review.
Please don't be put off comments on code style and indentation. But I like to be a bit picky here, since I really much like a code base that follows the same style (Even though I did not put up a style guide).
Your pull request is still really appreciated.

For the UInputDevice class please see comments about making it a member instead of a Singleton - I want to avoid Singletons as much as global variables. Use them only if absolutely necessary.

src/projecteurapp.cc

src/settings.cc

src/settings.h

src/spotlight.cc

jahnf · 2019-11-18T06:54:06Z

src/uinputevents.h

+
+using namespace std;
+
+class uinputEvents;


Not necessary.

src/uinputevents.h

src/uinputevents.cc

mayanksuman · 2019-11-18T07:32:04Z

Thanks for quick review. I will be able to resolve them in the evening (IST).

jahnf

See individual comments. Especially the memory leak.

jahnf · 2019-11-18T10:37:22Z

src/spotlight.cc

@@ -87,15 +88,35 @@ namespace {
 Spotlight::Spotlight(QObject* parent)
  : QObject(parent)
  , m_activeTimer(new QTimer(this))
+  , m_clickTimer(new QTimer(this))
+  , m_virtualDevice(new VirtualDevice)


Memory leak, object will never get destroyed. m_virtualDevice member should be of type QScopedPointer or std::unique_ptr

jahnf · 2019-11-18T10:38:59Z

src/spotlight.h

@@ -62,4 +66,7 @@ class Spotlight : public QObject
  std::map<QString, QScopedPointer<QSocketNotifier>> m_eventNotifiers;
  QTimer* m_activeTimer;
  bool m_spotActive = false;
+  bool m_clicked = false;
+  QTimer* m_clickTimer;
+  VirtualDevice* m_virtualDevice;


Memory leak. m_virtualDevice member should be of type QScopedPointer or std::unique_ptr

jahnf · 2019-11-18T10:40:12Z

src/virtualdevice.h

+class VirtualDevice{
+  private:
+    struct uinput_user_dev uinp;
+    static int uinp_fd;


~~static~~ , should be a regular member

int uinp_fd = -1;

jahnf · 2019-11-18T10:42:50Z

src/virtualdevice.cc

+
+// Setup uinput device that can send mouse and keyboard events. Logs the result too.
+void VirtualDevice::setupVirtualDevice() {
+  int i=0;


c-style ... remove .. use for (int i = 0; .... instead

jahnf · 2019-11-18T10:43:28Z

src/virtualdevice.cc

+  ioctl(uinp_fd, UI_SET_RELBIT, REL_X);
+  ioctl(uinp_fd, UI_SET_RELBIT, REL_Y);
+
+  for (i=0; i < 256; i++) {


use for (int i = 0; .... instead

src/virtualdevice.cc

jahnf · 2019-11-18T11:08:43Z

src/spotlight.cc

+      char msg[] = "A virtual device was not created. "\
+                   "Some features like changing pointer modes might not work.\n\n"\
+                   "Please check if uinput kernel module is loaded.";


This won't be picked up by Qt's lupdate tool.
Use const auto msg = tr("..."); and then the msg variable directly for QMessagebox

jahnf · 2019-11-18T11:09:45Z

src/virtualdevice.h

+#include <linux/uinput.h>
+#include <unistd.h>
+
+#include <QDebug>


No need to include this in the header

jahnf · 2019-11-18T11:28:40Z

src/virtualdevice.h

+class VirtualDevice{
+  private:
+    struct uinput_user_dev uinp;
+    int uinp_fd;


set default value

int uinp_fd = -1;

jahnf · 2019-11-18T11:36:49Z

src/virtualdevice.cc

+  if (ioctl(uinp_fd, UI_DEV_CREATE)) {
+    qDebug("Unable to create Virtual (UINPUT) device.");
+    deviceCreated = false;
+    return;
+  }


Close uinp_fd, otherwise it will not get closed.
Alternative: check in constructor if uinp_fd >= 0 and close

jahnf · 2019-11-18T11:53:28Z

I'd rather would merge your changes to a feature branch and then after all bugs and major issues are resolved merge it to develop . Even though it's develop I'd like it as stable as possible - and I am most likely will overlook an issue like the memory leak or sth else in a code review. I will create a feature branch , could you then please create a pull request into that new branch?

jahnf · 2019-11-18T11:59:27Z

Please make the pull request for feature/pointer-mode-uinput-dev instead of develop.

mayanksuman · 2019-11-18T13:09:21Z

Ok, I am making a pull request to feature/pointer-mode-uinput-dev.

Zoom Toggle on spotlight double click

85db4bc

mayanksuman mentioned this pull request Nov 17, 2019

Feature parity with official Logitech software #38

Closed

mayanksuman added 5 commits November 18, 2019 07:56

Minor changes

ad74ad2

Better qDebug messages about device connet/disconnect

e1096e5

Minor change in emitEvent function

b20cd2e

Got Rid of nested if logic in spotlight.cc

134cbfb

Added a option to retain timestamp of events in emitEvent

97edf93

mayanksuman changed the title ~~Presenter mode (Zoom) changes on double click (same behavior as of official software)~~ Pointer mode (Zoom) changes on double click (same behavior as of official software) Nov 18, 2019

mayanksuman added 5 commits November 18, 2019 09:44

Better member names for spotlight.h

49a6358

Minor changes in uinputevents.h

b6a485d

Merge branch 'develop' of https://github.com/jahnf/Projecteur into de…

936e9d9

…velop

setup_uinputDevice is private method of uinputEvents class

fa435c1

Fixed SpotModeChange in main.qml

64dd8ef

Removed Double Click event from main.qml

7b6a168

jahnf reviewed Nov 18, 2019

View reviewed changes

mayanksuman added 3 commits November 18, 2019 15:48

VirtualDevice Class is not a singleton anymore

c3307d0

Mostly code style changes

1d328a7

Minor code style changes

ba22b6c

jahnf requested changes Nov 18, 2019

View reviewed changes

jahnf reviewed Nov 18, 2019

View reviewed changes

mayanksuman closed this Nov 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pointer mode (Zoom) changes on double click (same behavior as of official software) #54

Pointer mode (Zoom) changes on double click (same behavior as of official software) #54

mayanksuman commented Nov 17, 2019 •

edited

jahnf commented Nov 18, 2019 •

edited

mayanksuman commented Nov 18, 2019

jahnf left a comment •

edited

jahnf Nov 18, 2019

mayanksuman commented Nov 18, 2019

jahnf left a comment

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf Nov 18, 2019

jahnf commented Nov 18, 2019

jahnf commented Nov 18, 2019

mayanksuman commented Nov 18, 2019

Pointer mode (Zoom) changes on double click (same behavior as of official software) #54

Pointer mode (Zoom) changes on double click (same behavior as of official software) #54

Conversation

mayanksuman commented Nov 17, 2019 • edited

jahnf commented Nov 18, 2019 • edited

mayanksuman commented Nov 18, 2019

jahnf left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayanksuman commented Nov 18, 2019

jahnf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahnf commented Nov 18, 2019

jahnf commented Nov 18, 2019

mayanksuman commented Nov 18, 2019

mayanksuman commented Nov 17, 2019 •

edited

jahnf commented Nov 18, 2019 •

edited

jahnf left a comment •

edited