-
-
Notifications
You must be signed in to change notification settings - Fork 994
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 context menu automation items #7317
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see quite a few beginner mistakes here. Pointed out those. Nothing major, just bad practices. Also look into the codefactor blank line one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need that else there?
There's lots more nesting going on. I am a bit tired to nitpick everything |
Feel free to nitpick. I'm open to learn new things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass of changes that should be made.
EDIT: Looks like my page didn't refresh with the latest commits for some reason. Not sure why. 🤔
Because I pushed the changes right before when you completed your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More nitpicks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
How should the new tracks be named? Currently it uses the default names. |
Btw forgot to say, this could fall in scope for #7027
I believe it should be named after whatever knob the automation is created from |
Please note that nowhere did I mention that I meant the current automation editor to be this first and preferred way. When I wrote this I had Bitwig's automation in mind. In Bitwig you can for example wiggle a control in a plugin and it will show as the current parameter in the automation track of that instrument track. You can then directly click points, move them, add tension to curves, etc. It just works straight forward and so far there never was a need to dive into context menus to accomplish something. Of course you can also manually select parameters or show all automation tracks of parameters that are automated.
Yes, I agree that the current workflow and organization of automation is "sub-optimal". |
Sorry I totally misunderstood you. I thought you said exactly the first sentence, sorry. |
I have fixed undo, added a new context menu option to open the closest clip and removed the remove option removing clips (but now it must leave at least one node in the clip to prevent some issues). |
Some things I have observed while checking the current version of the branch:
To be honest, all these features seem totally confusing to me. I don't understand how it should be feasible and helpful to the users to remotely add nodes to automation clips that they might not even see while they are interacting with context menus in controls. I am not really "dog-fooding" on LMMS and seldom do something with the automations. So it should be a good test to see how easy or hard it is to understand this feature. And for me it is hard to understand. Therefore I'd like some other @LMMS/developers to chime in here and take the decision with regards to whether this functionality should be added or not. @szeli1, I propose to wait for the result of the discussion with the other developers before any further reviews are done. |
I will fix this.
I thought you only wanted the auto clip deletion removed (which was removed, now if all the nodes get deleted the clip will not be removed). I would like to hear what other devs say about this.
Could you please describe to me exactly what are you trying to do and what do you expect the result to be? I don't know how could this happen and to me it seems like I did not give a good description of the features or there could be bugs. Edit: I have rewrote the PR description to make the context menu options easier to understand. |
Reading the updated description I think it's a bit clearer to me what you want to achieve. I was not aware that the playhead plays an important role with regards to that feature. Nevertheless I will describe what I expected without have this description available as this will be the situation most users will be in when they interact with the feature. Open automation clipSteps to reproduce:
Instead of opening the existing clip a new clip is added and opened. I'd expect it to open the existing clip because IMO it's more likely that I want to edit it instead of a completely new one with no edits. Add automation nodeSteps to reproduce:
Instead of adding a new node like the label says the existing node is changed. I'd expect a node to be added. |
Can not be fixed. You can not undo track creation. |
This is a bug which can be fixed. I will try to fix it.
This is an Thanks for that comment, it is in great detail (as I have requested). I think documentation would need to be rewritten so the users will be able to understand the options better. |
I tried this and although I didn't have all the time to read your comments I would like to quickly contribute some ideas like:
I don't know if I'm wrong or how difficult it would be to apply these changes, but without a doubt these suggestions would work better with all clips and their nodes, because for now those options only affect the closest clips and their first node, and that is useless when a knob has many clips and when they have many nodes, I did the test. (Google Translate) |
This PR is built on the playback position. Your suggestion doesn't. This PR doesn't aim to add new features, but to make existing features faster to use. I think the
This isn't supported and this PR doesn't touch the related files. You could make a new feature request about this.
Currently the option opens the closest clip before. I can see why this would be an issue. Usually users would like to playback the section which they are working on, so the playback position is almost always before where the automation clip would start. I will change the code so It opens the appropriate clip (Edit: Done). I can implement your suggestion instead if more people support it.
This is again an automation editor feature suggestion. It is not related to this PR.
Your suggestions are valid. There should be a way to edit multiple nodes at the same time, but I think this PR's goal isn't to mass edit multiple nodes, it is to make simple quick edits faster by using the playback position. |
I would like to provide my own opinion on the PR. Firstly, I have tested the options and can confirm everything works as intended. I would still like to provide a list of features and what they do, explained in a way better fit for the end user:
NOTE: If any parameter has more than one clip, the one closest to the top is selected. Secondly, I believe these would have to be appropriately documented, as they are quite confusing out-of-the-box. A YouTube video might do just as well, if not better, because this feature is best explained in-action. Thirdly, my honest opinion on the feature. I do not think this makes a big impact on the speed of editing automation. I tried making a slow sweep (0-25-100) using both methods. testing-cssli-cmenu.mp4testing-cssli-traditional.mp4I have not seen a notable difference, time-wise. Granted, this kind of test doesn't do justice to the other features, but I still do not think these make a noticable impact, given how they invite more work relating to documenting the feature. I would invite some more testers to form their opinions, since the feature is really nicely made, but just seems a bit redundant to me. |
Thanks for testing!
This is not always true and I don't know what you mean by "closest to the top".
I think it helps window management.
Thanks for making this description. I think it is better than my description. |
What I mean to say is "the one that appears first when traversing all automation tracks from top to bottom in the song editor".
My pleasure! |
closes #3112
This PR adds new context menu options for
automatableModels
to improve the automation workflow.This PR aims to make quick edits to automation tracks easier and faster to do. It does not aim to replace the automation editor.
These options are the following:
AutomationTracks
that have at least one clip. All clips have to be connected to the model. If a track with these requirements can not be found a new track will be added. After it found a track it will search for a clip before the playback position. If it can not find a clip it will add a new clip. After it found a clip it will add a newAutomationNode
to the clip at the playback position. The node's value will be equal to theAutomatableModel
's value. The node's placement depends on the quantization setting insideAutomationEditor
. If there is a clip before the playback position a new clip won't be created.AutomatableModel
's current value.AutomationNode
to the playback position. It will not delete clips and it will leave at least oneAutomationNode
inside a clip (because ofAutomationClip
limitations).AutomationNode
it will open the found / created clip in theAutomationEditor
. It will try to open the closest clip, it 2 clips are after each other and the playback position is (1/2 bar) close to the second clip then it will open the second clip.In my view these options will make the automation system faster to use.