new GPS text filter UI

[dany123]

[ for https://github.com/mltframework/mlt/pull/703 ]
This is the ui I came up with to fill out all filter properties.

I took a very C-like procedural approach with this, I'm not familiar either with Qt or javascript so feel free to offer advice/fixes.
There are some todos in the code but none of them are major.
As I said in the backend code, I had issues with just declaring variables at the top and not losing their values after minimising the shotcut window so I use filter.set("x") to reliably store values. This is also why there's some parseInt(Number(x)) there - to guard against NaN and empty values. If they look ugly I'll remove them and clean up but only after understanding/fixing the issue.

I got used to the way it looks, but maybe it's a bit too busy? I'm more of a "give me more options at the expense of nice interface" kind of guy and I'm not sure how to cut down on the options, the filter is also quite complex so it needs quite a bit more fields than usual. The descriptions (on hover) are also quite long, but this is to counter the complexity, I feel like someone with no prior experience should understand everything just by reading the hover tips.

[dany123]

There's one annoying behaviour that I need to fix with javascript, when i filter.set a larger number ending in 0 it will switch to scientific notation so something like -8850800 becomes:
-8.8508e+06
Edit: fixed in a150642

[bmatherly]

Fantastic job on this new filter! It has come a long way.

For status values like video and GPS start times, we tend to put those at the end below a white separator bar. See Compressor and Time Remap filters for example.

Can you explain "Insert GPS Field"? What is that for?

[bmatherly]

In general, I still do not understand major and minor offsets. And why can minor only be set in seconds but major also has seconds?

[dany123]

For status values like video and GPS start times, we tend to put those at the end below a white separator bar. See Compressor and Time Remap filters for example.

I wonder if that's the best decision here, putting them at the bottom would place them outside screen (at least on my 1080p display I need to scroll to get to the end) and they are quite important for sync issues.

Can you explain "Insert GPS Field"? What is that for?

It's the list of available keywords as a drop-down, it's the equivalent "Insert field" button list from Text: simple, I had to do a drop down list as there are 14 options

[dany123]

In general, I still do not understand major and minor offsets. And why can minor only be set in seconds but major also has seconds?

I'll try to explain in short: both are in seconds and both are independent of one another, technically you could remove any of them and still sync with no problems (of course, after lifting the 60s limit on the minor one) as in the backend I just add them together and use it as only one value in calculations. If there was an easy way to keep the total value into only one filter property I'd switch to it immediately, but for the UI, the 2 values are tied to the 2 separate ways to enter the ofset. (I assume the main problem is the 2 filter properties that are also sent to backend, not the fact that there are 2 Ui elements dealing with sync, or is it both?)

In the begining there was only one of them, the "major_offset" which is converted from seconds into days/hours/min/sec separately and printed individually so it is very clear how much offset there is. And it is enough for the best case where there the times are synced perfectly or they differ only by the timezone, so if the offset is 3600 seconds it will just show 00 01 00 00 and that's it.

But, most of the time this is not the case, unless the device has internet access and auto time sync it will probably be manually set to up to 1 min accuracy (as an example a gopro will only let you set HH:MM, not seconds, so it is almost guaranteed to have some error). So what do you do? you start inputting 1 second into the second textArea, you check to see if it's better on the overlay, maybe try 5, 10, 15, etc, but this is not efficient as you have to click into the box, replace the old number then click outside (or hit tab) to finish editing and have the text update on overlay.
This is where the slider spinner of the "minor_offset" comes into action, as if you use the mouse scrollwheel on it (or just click and drag), you can immediately go through 2 minutes of gps data and have it continously update it over the video so you can quickly check where the speed/altitude or something else matches the video at that point (so speed 0 if you stopped, or max altitude if you are at the peak of some mountain/track).

In practice I almost always used the minor offset slider instead of entering data manually into the fields. This is why I'm defending this separate minor/major split.

[bmatherly]

For status values like video and GPS start times, we tend to put those at the end below a white separator bar. See Compressor and Time Remap filters for example.

I wonder if that's the best decision here, putting them at the bottom would place them outside screen (at least on my 1080p display I need to scroll to get to the end) and they are quite important for sync issues.

There are tradeoffs in all UI decisions. I tent to prefer consistency in UI unless there is an exceptional reason to be inconsistent. If you move the status to the end under a white bar like the other filters, would it help to also display those statuses in a tooltip when you mouse over the file name? Maybe that would achieve consistency and convenience.

It's the list of available keywords as a drop-down, it's the equivalent "Insert field" button list from Text: simple, I had to do a drop down list as there are 14 options

It is not consistent with the dynamic text filter, but I understand the exceptional need here. Obviously we will not put 14 buttons. I kind of prefer the approach. Maybe we will change the dynamic text filter to match this someday.

In practice I almost always used the minor offset slider instead of entering data manually into the fields.

You should be able to use the wheel mouse to quickly "spin" or "step" the seconds value in the major offset. If the only reason to have two offsets is so that you can use two different controls, then let's find a control that can achieve both uses reasonably well.

The Timer filter uses the ClockSpinner.qml control. You could copy/paste that and modify it, or share the control and extend it to meet your needs. If you were to share it, you would need to:

1. Move the filter to the modules/Shotcut/Controls folder
2. Add an option to display/control days (needed for gps text)
3. Add an option to display/control miliseconds (needed for Timer)
4. Add a scroll wheel handler to increment seconds

[dany123]

There are tradeoffs in all UI decisions. I tent to prefer consistency in UI unless there is an exceptional reason to be inconsistent. If you move the status to the end under a white bar like the other filters, would it help to also display those statuses in a tooltip when you mouse over the file name? Maybe that would achieve consistency and convenience.

That would work for me, seems like a good compromise.

You should be able to use the wheel mouse to quickly "spin" or "step" the seconds value in the major offset. If the only reason to have two offsets is so that you can use two different controls, then let's find a control that can achieve both uses reasonably well.

The Timer filter uses the ClockSpinner.qml control. You could copy/paste that and modify it, or share the control and extend it to meet your needs. If you were to share it, you would need to:

1. Move the filter to the modules/Shotcut/Controls folder

2. Add an option to display/control days (needed for gps text)

3. Add an option to display/control miliseconds (needed for Timer)

4. Add a scroll wheel handler to increment seconds

Good idea, combining the controls should keep the functional part of both.

I am not a fan of the ClockSpinner in general, I mean it's ok for something standard like HH:MM:SS to read, but for editing if I need to change something in the middle (say 01:00:45 to 01:23:45) I find it faster to delete everything and just type the entire string at once.
Instead, I like the DoubleSpinBox functionality and design so I'll give this a shot to see if it can do all the stuff + if it fits the design.

Is there a simple onWheelUp/Down event for the simple textArea I can fallback to if the spinbox doesn't work as expected?

---

Update:

I did some tests today to check if doubleSpinBoxes would work and while they could be ok, if resized to a small size to fit on one line (width: 40) I can't edit the text, I mean at all, not even by focusing on the area with tab and pressing a number, I think only the outermost element or something is selected instead of the textfield. If I increase the width to 60 it seems to work.

(wip example:)

But I also have some issues with the limited acces to wheel events so I went to my backup plan to add mouse wheel functionality to the original textareas and after some searching I found something that works and is very flexible (I mean I can do the full 59 seconds +1 logic that increases correctly the next field). Would this solution work for you? I know the clockspinner would be the more consistent choice but there's a lot of changes needed for it and it would not be as functional as this choice.

Code looks like this (inside textItem):

TextField {
id: offset_hours
...
MouseArea {
	anchors.fill: parent
	onWheel: 
		if (wheel.angleDelta.y>0) 
		{ 
			offset_hours.text = Number(offset_hours.text)+1;  //TODO: time logic
			recompute_major_offset(); 
		}
		else  
		{ 
			offset_hours.text = Number(offset_hours.text)-1; 
			recompute_major_offset();       
		}
	onClicked: { offset_hours.forceActiveFocus() } //this seems to fix the issue of not allowing text editing below mousearea
	}
onFocusChanged: if (focus) selectAll()
}

[bmatherly]

But I also have some issues with the limited acces to wheel events so I went to my backup plan to add mouse wheel functionality to the original textareas and after some searching I found something that works and is very flexible (I mean I can do the full 59 seconds +1 logic that increases correctly the next field). Would this solution work for you?

It sounds promising and you are on the right track. We would want all the numeric fields to behave the same. So either duplicate the mouse event for each field, or make this into a new component.

[dany123]

I've cleaned up the design quite a bit after applying the suggestions, only 3 rows at the top for the important stuff before the text comes in.
Should I keep the bold category labels? (GPS options, Text options, Advanced options - after correcting case)
I feel like there is a bit more spacing between my rows compared to other filters, any idea why? The "Text" near the big textbox is also misaligned for no reason.
There is a lot of space (margin/padding?) around the TextFilterUi, not sure if related to previous spacing issue, how can I remove it?
Should I bother aligning items? (to allign the GPS offset and smoothing rows there's one element with 39 px width, it was 40 originally but the slight 1 px misalignment was quite annoying to look at)

Also, are multiple simple commits or single complex ones prefferred?

[bmatherly]

Should I keep the bold category labels? (GPS options, Text options, Advanced options - after correcting case)

That is up to you. There are other filters that make similar groupings. If you keep the category labels, maybe "Advanced" should be "Speed"?

I feel like there is a bit more spacing between my rows compared to other filters, any idea why?

It looks like you have a GridLayout inside a GridLayout. Did you do that on purpose? I think it should not be necessary. My recommendation is to group each row of components together in the code (with an empty line between each group) and then look very carefully at the use of columnSpan, etc.

For the padding around the TextArea, you might need to play with the padding and anchors. Keep in mind that the code you copied might be trying to reserve some space for scrollbars to appear if/when they are needed.

Also, are multiple simple commits or single complex ones prefferred?

You can keep adding commits to this review and we will squash them all before merging to master.

[dany123]

If you keep the category labels, maybe "Advanced" should be "Speed"?

Speed it is, I had some extra fields I planned to put there not related to speed but I scratched them for now, I might switch back to Advanced options if I ever add them to backend.

It looks like you have a GridLayout inside a GridLayout. Did you do that on purpose? I think it should not be necessary

At one point I tried aligning all buttons to the right using relative positioning, didn't work out and the grid was leftover. Changed in latest commit to a single rowlayout for that entire row.
I found out why the weird large spacings appeared: there was too much space in the main element (height: 800) so items spread around to fill it, lowered it to 700 and added a fillHeight element at the end and now it's good.

I also completely removed minor offset (will update backend soon). I'm not sure if it's worth to rename all "major_offset" references to something generic now.

[bmatherly]

I'm not sure if it's worth to rename all "major_offset" references to something generic now.

Maybe "time_offset" or just "offset"?

If we want to change it, it should be done now because it is difficult to change later after applications have started to depend on it.

[dany123]

Good point, I'll better change it now rather than later.

[dany123]

Is there an equivalent of producer.duration / profile.fps but for the original file duration?
Also I just noticed my tooltips are inconsistent regarding ending with a dot, should I add it to all sentences or remove it everywhere?

[bmatherly]

Is there an equivalent of producer.duration / profile.fps but for the original file duration?

Maybe try producer.length / profile.fps
https://github.com/mltframework/shotcut/blob/master/src/qmltypes/qmlproducer.h#L36
This is a recent addition, so you may need to checkout the latest master and rebase

Also I just noticed my tooltips are inconsistent regarding ending with a dot, should I add it to all sentences or remove it everywhere?

Prefer complete sentences with proper capitalization and punctuation.

[dany123]

Maybe try producer.length / profile.fps

This worked perfectly.