-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
new GPS text filter UI #1120
new GPS text filter UI #1120
Conversation
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: |
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? |
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 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.
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 |
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. 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. |
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 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.
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:
|
That would work for me, seems like a good compromise.
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. 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. 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):
|
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. |
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. Also, are multiple simple commits or single complex ones prefferred? |
That is up to you. There are other filters that make similar groupings. If you keep the category labels, maybe "Advanced" should be "Speed"?
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.
You can keep adding commits to this review and we will squash them all before merging to master. |
…parated filter.set from setControls, separate gps_setControls, moved info at the bottom, cleaned up layout, add button for gps_processing_now
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.
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 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. |
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. |
Good point, I'll better change it now rather than later. |
Is there an equivalent of |
Maybe try producer.length / profile.fps
Prefer complete sentences with proper capitalization and punctuation. |
This worked perfectly. |
[ 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.