Bug 356493 - [Patch included] "Save Layout As" menu option in menu bar redundant? Suggest removing "Save Layout As" sub-menu text
Summary: [Patch included] "Save Layout As" menu option in menu bar redundant? Suggest ...
Status: RESOLVED FIXED
Alias: None
Product: kdenlive
Classification: Applications
Component: User Interface (show other bugs)
Version: git-master
Platform: Ubuntu Linux
: NOR wishlist
Target Milestone: ---
Assignee: Jean-Baptiste Mardelle
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-10 23:47 UTC by Unknown
Modified: 2020-07-27 18:04 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 20.08
fritzibaby: Brainstorm+


Attachments
layoutmanagement.cpp file with patched code to fix the corresponding bug. (4.79 KB, text/x-c++src)
2017-08-15 00:45 UTC, Unknown
Details
Screenshot of layouts in shortcut menu after patch (14.43 KB, image/png)
2017-08-15 16:33 UTC, Andrew Shark
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Unknown 2015-12-10 23:47:14 UTC
When you customize the Kdenlive layout, then go to the top menu bar, click on View > Save Layout As > ... , it shows "Save as Layout 1", "Save as Layout 2", etc.

Loading the Layout, however, doesn't have "Load Layout 1", "Load Layout 2", etc. 

Consider the following changes:
1. Remove "Save as Layout..." from "Save Layout as" submenu, simply replacing them with "Layout 1", "Layout 2", etc.
2. Change "Save Layout as" in the View menu to simply "Save Layout", similar to the "Load Layout" option beneath it.

Reproducible: Always




Tested on Kdenlive 15.11.90+48 from vpinon's Wily ppa. Using Ubuntu GNOME 15.10 w/ Gnome 3.16.
Comment 1 Andrew Shark 2017-03-06 22:38:13 UTC
You can just remove "Save As " from kdenlive/src/layoutmanagement.cpp on line 34.
But I am not sure if current msgid "Save As Layout %1" will be automatically removed from po files of translation teams.
Comment 2 Andrew Shark 2017-03-16 17:55:31 UTC
Please, see this patch on review board: https://git.reviewboard.kde.org/r/130008/
Comment 3 Unknown 2017-04-12 22:49:28 UTC
Thanks for submitting that patch, Ashark. I'd actually performed that commit on my forked github repo, but I guess that I didn't know the whole steps to submit the patch for review. :) Still so much to learn! Hoping to hear back from the Kdenlive team -- would love to say I'd committed to the project beyond bug testing.
Comment 4 Andrew Shark 2017-04-12 23:51:29 UTC
Hello, Jesse.
Github is a read only mirror of KDE repositories. See this page: https://community.kde.org/Infrastructure/Github_Mirror

But I saw your patch, and I think my is more "optimised".
On line 69 you keep i18n("%1", layoutName)); to send a string.
However, in this case i18n will just return %1 and then replace it with layoutName and after that this string could be used by setText.
In my patch, I just give layoutName to setText:
saveActions[j]->setText(layoutName);

I suggest you to add [patch provided] to the title of this bug report and then it will be easy for team to spot and apply this patch.
Comment 5 Andrew Shark 2017-04-12 23:53:20 UTC
By the way, this bug is screenshoted in features page in main site: https://kdenlive.org/features/
Comment 6 Unknown 2017-08-15 00:45:57 UTC
Created attachment 107280 [details]
layoutmanagement.cpp file with patched code to fix the corresponding bug.
Comment 7 Unknown 2017-08-15 00:47:34 UTC
@Ashark, thanks for the info'.

Alright, so I've been teaching myself C# and C++ (primarily for VR game design). I'm still a novice, but I feel I've sufficiently modified the code in layoutmanagement.cpp and would like to submit a patch for review. This is my first time submitting a patch, so feel free to correct me on what I should/shouldn't be doing.

I'd recently attached a modified layoutmanagement.cpp file with the code changes for review and approval. How do things proceed from here?
Comment 8 Unknown 2017-08-15 00:48:02 UTC
@Ashark, thanks for the info'.

Alright, so I've been teaching myself C# and C++ (primarily for VR game design). I'm still a novice, but I feel I've sufficiently modified the code in layoutmanagement.cpp and would like to submit a patch for review. This is my first time submitting a patch, so feel free to correct me on what I should/shouldn't be doing.

I'd recently attached a modified layoutmanagement.cpp file with the code changes for review and approval. How do things proceed from here?
Comment 9 Jean-Baptiste Mardelle 2017-08-15 07:09:08 UTC
My only concern with this is that if I remember correctly, loading and saving layouts can be assigned shortcuts. Currently, the "save as" part in the name helps differentiate between the load and save actions in the shortcut dialog. If we remove "save as", i am afraid both load and save actions will appear under "layout 1" in the shortcut dialog. Can uou check that ?
Comment 10 Andrew Shark 2017-08-15 16:30:42 UTC
Yes, layouts will appear without differentiate. See attached screenshot.
Comment 11 Andrew Shark 2017-08-15 16:33:45 UTC
Created attachment 107297 [details]
Screenshot of layouts in shortcut menu after patch

In shortcuts menu it will be difficult to differentiate actions for layouts.
Comment 12 Unknown 2017-08-15 22:48:16 UTC
Thanks JB and Ashark. I definitely see the concern, there. Give me a bit to see if I can come up with a creative solution.
Comment 13 Unknown 2017-08-15 22:48:53 UTC
(And sorry, the bug tracker isn't emailing me when a new comment appears on a bug I'm following. Not sure why, so I have to check bugs for new comments, manually.)
Comment 14 Unknown 2017-08-15 23:07:43 UTC
Ashark, you're right about the optimized code. I replaced "setText(i18n("%1", layoutName))" with "setText(layoutName). Apprecaite the guidance, there.
Comment 15 Unknown 2017-08-16 00:08:37 UTC
Could we possibly sub-categorize each of them in the Configure shortcuts window?

For example:
Layouts
- Save Layout
- - Layout 1
- - Layout 2
- - Layout 3
- - Layout 4
- Load Layout
- - Layout 1
- - Layout 2
- - Layout 3
- - Layout 4
Comment 16 Jean-Baptiste Mardelle 2017-08-16 06:18:37 UTC
Yes, I was also thinking about it. Shouldn't be hard.
in layoutmanagement.cpp at line 27, we gave:

KActionCategory *layoutActions = new KActionCategory(i18n("Layouts"), pCore->window()->actionCollection());

Which puts items in a "Layout" category. So instead we can simply create 2 categories: "Load Layout" and "Save Layout", and put the actions in the correct category.
Comment 17 emohr 2020-04-04 17:29:44 UTC
see here: https://invent.kde.org/kde/kdenlive/-/issues/407
Comment 18 emohr 2020-07-27 18:04:25 UTC
New layout function with preset layouts, customized layout and save as will come with 20.08.

We close this bug. If it still appears in the latest version, please feel free to re-open it and update the affected version number.