After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 622079 - Action to "Use this clip's parameters as project settings"
Action to "Use this clip's parameters as project settings"
Status: RESOLVED FIXED
Product: pitivi
Classification: Other
Component: User interface
Git
Other Linux
: Normal enhancement
: 0.91
Assigned To: Pitivi maintainers
Pitivi maintainers
: 582382 629145 (view as bug list)
Depends on:
Blocks: 630751
 
 
Reported: 2010-06-19 14:07 UTC by Jean-François Fortin Tam
Modified: 2013-09-01 01:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mockup of the Clip Properties tab (103.59 KB, image/png)
2011-01-31 13:15 UTC, Jean-François Fortin Tam
  Details
Proposed Patch (30.41 KB, patch)
2011-02-11 15:19 UTC, parthasarathi susarla
none Details | Review
mockup for the "show a dialog" approach (70.00 KB, application/x-tar)
2011-02-12 16:19 UTC, Jean-François Fortin Tam
  Details
reworked patch against the (latest) git master (28.66 KB, patch)
2011-02-14 11:25 UTC, parthasarathi susarla
none Details | Review
reworked patch against the (latest) git master (28.72 KB, patch)
2011-02-16 12:51 UTC, parthasarathi susarla
needs-work Details | Review
Patch (31.86 KB, patch)
2012-03-20 19:02 UTC, Matas Brazdeikis
needs-work Details | Review
Some more changes to patch (30.57 KB, patch)
2012-03-25 18:45 UTC, Matas Brazdeikis
none Details | Review
Fixed patch (38.98 KB, patch)
2012-03-25 22:13 UTC, Matas Brazdeikis
none Details | Review

Description Jean-François Fortin Tam 2010-06-19 14:07:25 UTC
There could be an item in the Project menu that says something like "Use this clip for project settings", or maybe a button in the project settings that says "Get settings from a clip..." that allows you to choose a "typical" clip that will set the project resolution, framerate, aspect ratio, sound settings, etc.
Comment 1 antistress 2010-06-19 15:17:46 UTC
i agree : it would make life much easier for a lot of users i think
Comment 2 Edward Hervey 2010-06-20 09:22:25 UTC
Some time ago we would set the project resolution to the resolution/parameters of the first clip added. Can't remember why we removed this though.
Comment 3 Jean-François Fortin Tam 2010-06-20 13:41:03 UTC
Probably because you can't trust the first added clip to be "representative" of what the desired project outcome would be; however, if the user explicitely asks for it, then you can be sure it is representative, I suppose.
Comment 4 Jean-François Fortin Tam 2010-11-28 14:19:07 UTC
*** Bug 629145 has been marked as a duplicate of this bug. ***
Comment 5 Jean-François Fortin Tam 2010-11-28 14:23:25 UTC
From comments 2 and 3 in 629145:

If the clip is:
- video+audio: set both audio and video settings
- video only: set video settings only
- audio only: set audio settings only
- a still image: set only the resolution
Comment 6 Jean-François Fortin Tam 2010-12-12 14:54:42 UTC
*** Bug 582382 has been marked as a duplicate of this bug. ***
Comment 7 parthasarathi susarla 2011-01-31 12:33:23 UTC
My idea is to create a 'Properties' menu (on the right-click pop-up menu) for items in the sources list which displays the Clip Properties and then the user can decide to Import those properties into the project. A screenshot:
http://people.collabora.co.uk/~partha/prop-1.png

The properties menu will be context specific, shows details based on the type of clip.
For a clip which has both audio and video streams: http://people.collabora.co.uk/~partha/Screenshot-Clip%20Properties-video+audio.png

For a clip which has just audio: http://people.collabora.co.uk/~partha/Screenshot-Clip%20Properties-audio.png

For a clip which is an image: http://people.collabora.co.uk/~partha/Screenshot-Clip%20Properties-image.png

I'm working on a patch for this feature, and I would appreciate any feedback on my idea on implementing this feature.
Comment 8 Jean-François Fortin Tam 2011-01-31 13:15:17 UTC
Created attachment 179717 [details]
mockup of the Clip Properties tab

Sounds like a good idea to me as it could eventually pave the way to bug 593919, bug 609971, etc. 

UI considerations: the "effect properties" tab was previously named "Clip properties" and contained expander widgets (one of which was "Effects").

The reason those expanders were removed and the tab renamed "Effects properties" is that, at that time, there was nothing else [than effect properties] in this tab.

So, instead of using separate dialogs for what you've shown in your screenshots, you could perhaps put that in an expander inside the "Effects properties" tab and rename the tab to "Clip properties".
Comment 9 antistress 2011-01-31 23:50:19 UTC
It's always surprising to see that there is no contextual menu at present time for clips in the library.
I often right click on a clip before remembering that.
Right clicking on a clip to get the option to set the project with its specification seems very natural to me
Comment 10 Jean-François Fortin Tam 2011-02-01 01:07:36 UTC
For the record: technically and visually, I think the two approaches (popup menu and "clip properties" tab) are not mutually exclusive.
Comment 11 parthasarathi susarla 2011-02-01 14:35:23 UTC
I'm not too sure if the properties should be clubbed with effect properties. I will probably go ahead and create a patch for that design too (I have the back-end code read, just need to fix up the UI interaction part) and attach both patches here, and maybe we can chose which one is the way to go ahead.
Comment 12 Alex Băluț 2011-02-02 08:33:54 UTC
The "Import Properties" button is very vague. It's not clear at all where exactly it will import those properties.

We could have, for example, in the Project -> Project Settings window, a "Drag clip here to use its properties" zone. It could be made to work when a clip is dragged to the window, not necessarily to this zone.

This would have some advantages:
- it's simpler to understand (the message is explicit), and
- the user can press Cancel if the settings are not good.
Comment 13 parthasarathi susarla 2011-02-02 08:54:09 UTC
'Drag Clip here' doesn't sound natural to me.  Although I agree that 'Import Settings' sounds vague.
IMHO, a clip properties dialog showing the clip properties and then a 'Save as Preset' button in the resulting dialog seems more natural and simple to use
Comment 14 Alex Băluț 2011-02-02 09:48:01 UTC
> 'Drag Clip here' doesn't sound natural to me.  
> Although I agree that 'Import Settings' sounds 
> vague.

"Drag a clip from the Media Library to use its properties as project settings"
Is this message more clear?

Unfortunately we use "clip" for both "clip in Media Library" and "clip in Timeline".

> IMHO, a clip properties dialog showing the clip 
> properties and then a 'Save as Preset' button 
> in the resulting dialog seems more natural and
> simple to use

IMO this is a good way to not clutter the yet-unexisting "(Media Library) Clip Properties" window, or the "(Timeline) Clip Properties" tab Jean-François mentioned.

In each scenario I thought of, at some point the user goes to the Project Settings window to (manually) set the project settings..
Comment 15 parthasarathi susarla 2011-02-11 15:19:57 UTC
Created attachment 180660 [details] [review]
Proposed Patch

The attached patch adds an menu item "Use Clip Properties" (on the right-click menu in the sources list). Clicking that will open a  "Import Properties" Dialog with shows the properties of the clip Audio/Video/Image (in tabs). The user then has the option of clicking OK or Cancel buttons. 

Clicking OK will import the settings into the project. And clicking Cancel will just close the dialog.

The UI (dialog box) looks a little primitive as of now, but I can surely change it anytime based on the feedback.

Currently, it only imports Audio and Video settings. Image settings although shown, are not imported. (Also, the Dialog doesn't look good at all when showing only the Image properties).

(Awaiting feedback)
Comment 16 Jean-François Fortin Tam 2011-02-12 16:19:05 UTC
Created attachment 180727 [details]
mockup for the "show a dialog" approach

Overall review: the UI needs some love. I'm attaching a mockup with screenshots comparing the current UI to my mockup (which eliminates tabs, makes text look much cleaner, and also provides a way for the user to selectively exclude parameters to apply to the project). My mockup makes it much more HIG compliant.


Also, in the current code, clicking the OK button causes this traceback:
  • File "pitivi/ui/clipparameters.py", line 148 in onUseSettingsButtonClicked
    if (self.is_video):
AttributeError: 'ClipParametersDialog' object has no attribute 'is_video'

Comment 17 parthasarathi susarla 2011-02-13 02:15:35 UTC
(In reply to comment #16)
> Created an attachment (id=180727) [details]
> mockup for the "show a dialog" approach
> 
> Overall review: the UI needs some love. I'm attaching a mockup with screenshots
> comparing the current UI to my mockup (which eliminates tabs, makes text look
> much cleaner, and also provides a way for the user to selectively exclude
> parameters to apply to the project). My mockup makes it much more HIG
> compliant.

The mockup looks amazing. Thank you. I shall attach an updated patch soon. 

> 
> 
> Also, in the current code, clicking the OK button causes this traceback:
> 
That is weird. I just checked that, and it works fine. Will test all possible combinations and see where i might be doing wrong.

Thanks for the review.
Comment 18 parthasarathi susarla 2011-02-14 11:25:26 UTC
Created attachment 180815 [details] [review]
reworked patch against the (latest) git master

The reworked patch, with changes and UI as suggested by nekohayo.
Comment 19 parthasarathi susarla 2011-02-16 12:51:06 UTC
Created attachment 180990 [details] [review]
reworked patch against the (latest) git master

hide the stray frames that appear when showing the Image properties. (the earlier patch still showed them)
Comment 20 Jean-François Fortin Tam 2011-02-24 16:29:28 UTC
With the latest patch, clicking the "Apply to project" button causes an AttributeError: 'ClipParametersDialog' object has no attribute 'is_video.

Clicking the cancel button yields another AttributeError: 'NoneType' object has no attribute 'destroy'.
Comment 21 Jean-François Fortin Tam 2011-05-24 17:23:46 UTC
ping Parthasarathi, can you fix your latest patch (and check if it still applies to master)?
Comment 22 Jean-François Fortin Tam 2011-06-22 00:55:19 UTC
Review of attachment 180990 [details] [review]:

The patch needs to be rebased to reflect the changes in master. 
The .glade file to be ported to gtkbuilder (.ui file).
Errors that happen when clicking the "Apply to project" and the "Cancel" buttons need to be fixed.

Once this is done, we will be happy to review your patch again!
Comment 23 Jean-François Fortin Tam 2011-06-22 01:00:08 UTC
Also, don't use syntax such as

if (self.is_video):

...But instead:

if self.is_video:


Also I noticed that a couple of strings are not marked for translation. For example, do _("my string") instead of simply "my string".
Comment 24 Jean-François Fortin Tam 2012-01-10 21:11:26 UTC
In the GES version of pitivi now, if your project and render settings match those of your input clips (including codec and resolution), a "smart render" occurs: pitivi does not need to re-encode your video. Rendering is then instantaneous and lossless.

The bug report here now gains an additional use case: setting the codecs and muxers to match the input clips.
Comment 25 Matas Brazdeikis 2012-03-20 19:02:36 UTC
Created attachment 210195 [details] [review]
Patch

Patch which allows to copy media settings to project settings.
Still needs to copy to render configuration. It is not so easy, while machine can not to have same codecs, etc.
Comment 26 Jean-François Fortin Tam 2012-03-25 13:59:09 UTC
Hi Matas, given the amount of changes that went in lately (code being removed/cleaned, files being renamed, etc.), could you rebase your patch onto origin/ges instead of origin/ges? (I know this isn't super trivial but it would be quite useful!)
Comment 27 Matas Brazdeikis 2012-03-25 14:55:49 UTC
Hey, it is patch for git origin/ges. Do you want patch for origin/master? (If yes why?)

Just checked
Git status - On branch ges.
git apply - works without any message.

Only one small mistake, after applying patch change pitivi/utils/ui.py:375, 10000000.0 to 1000000.0.
Comment 28 Jean-François Fortin Tam 2012-03-25 15:41:08 UTC
Review of attachment 210195 [details] [review]:

Oops, indeed I mistakenly thought your patch was based on master instead of ges. The patch behaves as expected, no bugs found. Remaining issues are:
- the small mistake you've identified
- the alignment of the checkboxes+labels
- try to reuse the exact same strings as the project settings window or other parts of the code (ex: the tooltip that shows up when you put the mouse over a clip in the media library) so that translators don't have to do any extra work for this.

Good work! You're almost there!
Comment 29 Matas Brazdeikis 2012-03-25 18:45:11 UTC
Created attachment 210587 [details] [review]
Some more changes to patch

Searched for strings. Done as much as posible.
Please look into it before commiting. Could be some small mistakes. Tested with video, audio and image.
Comment 30 Jean-François Fortin Tam 2012-03-25 20:58:30 UTC
Something's strange with your latest patch: it conflicts with the previous patch version, yet if you try to use it standalone it doesn't work either (it is incomplete, there's no menu item to show the clip properties etc.).
Comment 31 Matas Brazdeikis 2012-03-25 22:13:20 UTC
Created attachment 210595 [details] [review]
Fixed patch
Comment 32 Jean-François Fortin Tam 2012-03-27 04:02:04 UTC
Pushed to the main pitivi ges branch, with some minor amends to the original commit and an additional commit for refactoring. Thanks!


commit 02cbcafb01becbd256ef259a7ef7e47c8ef1a529
Author: Jean-François Fortin Tam <nekohayo@gmail.com>
Date:   Mon Mar 26 22:44:07 2012 -0400

    Refactor the clip media properties code

commit 98cd55ee867779c9cb57195f26b9d4302d023e0b
Author: Matas Brazdeikis <matas@brazdeikis.lt>
Date:   Sun Mar 25 23:11:10 2012 +0100

    Allow copying a clip's properties to project settings