0001893: Jog shuttle navigation improvement

(0006106)
ddennedy (developer)
2010-11-22 02:01

Few things wrong in this patch:
- indentation should 4 spaces only - not a mix of tabs and spaces and I saw a 2 space indent in other places.
- there are unrelated changes to LinuxCOM.h and renderer.cpp despite whatever warnings they may clear
- I can easily fix the above two myself, but the biggest problem is that it does not apply - every hunk failed!

Do you believe this change may break the jog/shuttle for which the original code is written? (I forget what that was, and I did not write the existing code.)

(0006107)
fleury (reporter)
2010-11-22 02:46

will fix the two issues (indentation and removal of the unrelated changes).
Note that with my version of gcc, the code would not compile without the virtual destructor uncommented though.

I think I know the problem of the hunks failing: git diff does not by default get the paths right. I will resubmit a new patch with these fixed too.

Will you take care of the commented virtual destructor ?

(0006108)
fleury (reporter)
2010-11-22 03:24

Can you try again with the new patch ?
Forgive me if this still does not apply, as I am not sure what relative paths should be used in there.

(0006109)
ddennedy (developer)
2010-11-22 04:13

Nevermind about the hunk failures, it does work with -p2; I was using -p1. It's not a git thing, but usually patch gives a different error when it can not locate the files. I will give it a test.

(0006111)
ddennedy (developer)
2010-11-22 09:33

The patch is working very well for me.
Now, for the buttons, see MainWindow::slotShuttleButton(). Why is it mapping from 5 instead of 1? (I know that is not your code.)

(0006112)
ddennedy (developer)
2010-11-22 09:39

Re previous comment, button 1 in the settings is triggered by button 5 on the Contour ShuttlePro.

(0006113)
ddennedy (developer)
2010-11-22 09:52

In MainWindow::slotShuttleAction() I find this handy:

+ case 2:
+ slotSwitchMonitors();
+ break;

Note: the "Cut" action is not currently implemented. A cut action should call slotCutTimelineClip().

(0006114)
fleury (reporter)
2010-11-22 10:15

Oh, I thought I did not change this button behavior. In any case, for me the buttons were more or less useless, so I did not (yet) bother.

For that (button mapping), I am preparing another patch to have a better mapping from jogshuttle events to actual actions. I can elaborate on the path I want to take on that, but probably not in this bug.

(0006115)
ddennedy (developer)
2010-11-22 22:01

Re the commented virtual destructor in LinuxCOM.h. I agree it is correct to uncomment that, and I was surprised to see it, but here is the SVN commit that resulted in that:
http://kdenlive.svn.sourceforge.net/viewvc/kdenlive?view=revision&revision=4978 [^]

(0006116)
fleury (reporter)
2010-11-23 00:02

I guess the ShuttlePRO has the same numbering of buttons as my ShuttlePROv2. In this case, the row at the top are the buttons 1 (left) to 4 (right), then the second row a bit closer to the shuttle are buttons 5 to 9.
On my machine, the settings are working as expected.

(0006117)
ddennedy (developer)
2010-11-23 03:02

In ShuttleThread::key() the first key (top left, as you describe) code is 256. Due to MainWindow::slotShuttleButton()'s switch going from 5 - 9, that makes the top row of buttons unusable. Put a fprintf in ShuttleThread::key() and see what code you get for the top row of buttons.

(0006118)
ddennedy (developer)
2010-11-23 20:44

Per JB's confirmation, I can commit this patch. Is there an update or anything else you wanted in this patch before I go ahead?

(0006119)
ddennedy (developer)
2010-11-24 04:53

I noticed a bug:
Object::connect: No such slot MonitorManager::stop()

Since the patch has been working without JogShuttle::stop successfully connected to anything, the solution is to simply remove that line from mainwindow.cpp.

(0006120)
ddennedy (developer)
2010-11-24 04:57

Applied patch to svn commit 5115.

Issue History
Date Modified	Username	Field	Change
2010-11-21 21:58	fleury	New Issue
2010-11-21 21:58	fleury	File Added: shuttle_speeds.patch.gz
2010-11-22 02:01	ddennedy	Note Added: 0006106
2010-11-22 02:02	ddennedy	Assigned To	=> ddennedy
2010-11-22 02:02	ddennedy	Status	new => feedback
2010-11-22 02:46	fleury	Note Added: 0006107
2010-11-22 02:46	fleury	Status	feedback => assigned
2010-11-22 03:23	fleury	File Added: shuttle_speeds_v2.patch.gz
2010-11-22 03:24	fleury	Note Added: 0006108
2010-11-22 04:13	ddennedy	Note Added: 0006109
2010-11-22 09:33	ddennedy	Note Added: 0006111
2010-11-22 09:39	ddennedy	Note Added: 0006112
2010-11-22 09:52	ddennedy	Note Added: 0006113
2010-11-22 10:15	fleury	Note Added: 0006114
2010-11-22 22:01	ddennedy	Note Added: 0006115
2010-11-23 00:02	fleury	Note Added: 0006116
2010-11-23 03:02	ddennedy	Note Added: 0006117
2010-11-23 03:04	ddennedy	Relationship added	has duplicate 0001892
2010-11-23 03:05	ddennedy	Relationship added	has duplicate 0001891
2010-11-23 20:44	ddennedy	Note Added: 0006118
2010-11-24 04:53	ddennedy	Note Added: 0006119
2010-11-24 04:57	ddennedy	Note Added: 0006120
2010-11-24 04:57	ddennedy	Status	assigned => resolved
2010-11-24 04:57	ddennedy	Fixed in Version	=> 0.7.9
2010-11-24 04:57	ddennedy	Resolution	open => fixed
2011-04-26 21:57	j-b-m	Fixed in Version	0.7.9 => 0.8
2011-06-10 10:50	Granjow	Status	resolved => closed

Notes
(0006106) ddennedy (developer) 2010-11-22 02:01	Few things wrong in this patch: - indentation should 4 spaces only - not a mix of tabs and spaces and I saw a 2 space indent in other places. - there are unrelated changes to LinuxCOM.h and renderer.cpp despite whatever warnings they may clear - I can easily fix the above two myself, but the biggest problem is that it does not apply - every hunk failed! Do you believe this change may break the jog/shuttle for which the original code is written? (I forget what that was, and I did not write the existing code.)

(0006107) fleury (reporter) 2010-11-22 02:46	will fix the two issues (indentation and removal of the unrelated changes). Note that with my version of gcc, the code would not compile without the virtual destructor uncommented though. I think I know the problem of the hunks failing: git diff does not by default get the paths right. I will resubmit a new patch with these fixed too. Will you take care of the commented virtual destructor ?

(0006108) fleury (reporter) 2010-11-22 03:24	Can you try again with the new patch ? Forgive me if this still does not apply, as I am not sure what relative paths should be used in there.

(0006109) ddennedy (developer) 2010-11-22 04:13	Nevermind about the hunk failures, it does work with -p2; I was using -p1. It's not a git thing, but usually patch gives a different error when it can not locate the files. I will give it a test.

(0006111) ddennedy (developer) 2010-11-22 09:33	The patch is working very well for me. Now, for the buttons, see MainWindow::slotShuttleButton(). Why is it mapping from 5 instead of 1? (I know that is not your code.)

(0006112) ddennedy (developer) 2010-11-22 09:39	Re previous comment, button 1 in the settings is triggered by button 5 on the Contour ShuttlePro.

(0006113) ddennedy (developer) 2010-11-22 09:52	In MainWindow::slotShuttleAction() I find this handy: + case 2: + slotSwitchMonitors(); + break; Note: the "Cut" action is not currently implemented. A cut action should call slotCutTimelineClip().

(0006114) fleury (reporter) 2010-11-22 10:15	Oh, I thought I did not change this button behavior. In any case, for me the buttons were more or less useless, so I did not (yet) bother. For that (button mapping), I am preparing another patch to have a better mapping from jogshuttle events to actual actions. I can elaborate on the path I want to take on that, but probably not in this bug.

(0006115) ddennedy (developer) 2010-11-22 22:01	Re the commented virtual destructor in LinuxCOM.h. I agree it is correct to uncomment that, and I was surprised to see it, but here is the SVN commit that resulted in that: http://kdenlive.svn.sourceforge.net/viewvc/kdenlive?view=revision&revision=4978 [^]

(0006116) fleury (reporter) 2010-11-23 00:02	I guess the ShuttlePRO has the same numbering of buttons as my ShuttlePROv2. In this case, the row at the top are the buttons 1 (left) to 4 (right), then the second row a bit closer to the shuttle are buttons 5 to 9. On my machine, the settings are working as expected.

(0006117) ddennedy (developer) 2010-11-23 03:02	In ShuttleThread::key() the first key (top left, as you describe) code is 256. Due to MainWindow::slotShuttleButton()'s switch going from 5 - 9, that makes the top row of buttons unusable. Put a fprintf in ShuttleThread::key() and see what code you get for the top row of buttons.

(0006118) ddennedy (developer) 2010-11-23 20:44	Per JB's confirmation, I can commit this patch. Is there an update or anything else you wanted in this patch before I go ahead?

(0006119) ddennedy (developer) 2010-11-24 04:53	I noticed a bug: Object::connect: No such slot MonitorManager::stop() Since the patch has been working without JogShuttle::stop successfully connected to anything, the solution is to simply remove that line from mainwindow.cpp.

(0006120) ddennedy (developer) 2010-11-24 04:57	Applied patch to svn commit 5115.