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 586003 - Previewer in the importing file chooser
Previewer in the importing file chooser
Status: RESOLVED FIXED
Product: pitivi
Classification: Other
Component: User interface
Git
Other Linux
: Normal enhancement
: 0.14
Assigned To: Pitivi maintainers
Pitivi maintainers
Depends on:
Blocks: 650806
 
 
Reported: 2009-06-16 15:44 UTC by Jean-François Fortin Tam
Modified: 2011-05-22 17:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot (218.56 KB, image/png)
2010-05-23 01:18 UTC, Jean-François Fortin Tam
Details
screenshot when previewing fails (144.47 KB, image/png)
2010-08-27 14:20 UTC, Jean-François Fortin Tam
Details
Image displaying over-sized file chooser window (118.59 KB, image/png)
2011-04-28 17:32 UTC, Brandon Lewis
Details

Description Jean-François Fortin Tam 2009-06-16 15:44:27 UTC
It would be nice to have a small previewer (and a play/pause button and slider) when using the filechooser to add clips to a project.
Comment 1 antistress 2009-06-27 12:18:02 UTC
and also for pictures of course
Comment 2 antistress 2009-07-13 14:47:11 UTC
Please note that Braseor already has preview in the importing file chooser for pictures, songs and video.
It may be useful
Comment 3 Alessandro Decina 2009-07-13 15:14:55 UTC
yes, thank you for pointing it out
Comment 4 antistress 2009-07-14 08:35:07 UTC
see also that brasero bug : Allow user to zoom in/out in media previewer with mouse wheel ( http://bugzilla.gnome.org/show_bug.cgi?id=588406 )
Comment 5 Pier Carteri 2010-05-08 12:14:56 UTC
I start working on this...
You can find my git branch at 
http://github.com/m3tr0/pitivi---preview-on-filechooser

It's not complete now, but some hints from pitivi developers are much appreciate!
Comment 6 Jean-François Fortin Tam 2010-05-09 14:10:05 UTC
Hi, some quick UI feedback (from a non-dev) on your branch (you're probably already aware of those issues though):

- Ideally, it should show the frame (image) of the video while paused
- The slider bar doesn't work very well
- We should be able to use the slider bar to seek in the video without even using the play button
- AFAIK, gstreamer is able to do scrubbing with the video; for example, *while* you drag the slider in totem or pitivi's playhead, the video updates during the drag... your implementation doesn't do that, it changes the playhead position only when clicking and releasing the slider. Unless there's a performance issue at stake, it would be very nice to be able to add live "scrubbing" like that :)
- When switching from one clip to another, the slider is not reset (until you click Play)
- The "<b>Video</b>" text is redundant, we know it's a video :) but the resolution is nice to have. Or maybe you could add the info about audio channels for videos? (see below)
- There's a lot of wasted vertical space. Maybe it could be filled with metadata, like the source list's "list view", or even more. For example, we could also have artist/title/copyright/year information, while we're at it (and this could maybe be reused in a tooltip somewhere in the source list? hmm). This becomes especially important for audio files, too.
- For some reason, while it's all top-aligned for video files, it's stretched from top to bottom for audio files. I'd prefer that the two kinds of previewers are kept consistent.

Overall it does work, and you did some really great job!
Comment 7 Pier Carteri 2010-05-22 16:56:15 UTC
I've upload a new version of the code. This is a major rework that tries to fix bugs/missing features as pointed out 
from pitivi devs/users (thanks to Jean-François Fortin)

List of changes:
* Add zoom option on video playback (as in Brasero)
* Show metadata informations (tags)
* Improve slider usability: 
   you can seek without play the video
   you can "live scrubbing" 
* Better use of space in preview panel

TODO/BUGFIX
* still some problems on pause (the image is displayed but sometimes not completely)
* now all tags related to a clip are displayed in alphabetical order. This is not so useful (for example title is usually at the bottom...) but I don't know if is better to have a fix set of tags in a predefined order. Also related to this, the name of tags are defined somewhere? For example should I look for "author" or "artist" or something else?
Comment 8 Jean-François Fortin Tam 2010-05-23 01:18:27 UTC
Created attachment 161752 [details]
screenshot

Nice work! Big problem with the fact that you automatically use all the tags of a clip though... see this screenshot :)

If it's possible to restrict this to a manually-defined set of tags, it would be better. We should only care about a handful of tags for video, and another handful of tags for audio, I think (don't know if all the files provide them consistently though... and if they don't, you need to make sure you have stuff in place to handle the failing scenarios).

Also, clicking somewhere in the slider while the clip is playing yields an incorrect result (compared to clicking when it's paused, or scrubbing).
Comment 9 Edward Hervey 2010-05-23 08:56:49 UTC
For the tags you can get the human-readable (and translated) names by using gst.tag_get_nick(tag). Would make it cleaner.

You could also limit the tags by only showing those that are strings/numericals (sort using gst.tag_get_tag_type(tag)). I'd do it that way, rather than assuming that files will always produce certain tags.
Comment 10 Pier Carteri 2010-05-24 08:28:32 UTC
Thanks for feedback!
I will fix the tag problem following the Edward hints.
On the bug on slider found by Jean-François: 
Now when you click somewhere on slider, the cursor position is increased/decreased by a page_increment that is a 10% of clip duration (I'm using the standard behaviour for gtk.Scale). I will change the code to have the same behaviour as when the clip is not playng.

On play/stop issue: has someone encountered the same issue I mentioned before (i.e. when you pause the preview the image is not completely displayed)? If no maybe is a problem  on my installation (I'm on ubuntu 10.4 beta)
Comment 11 Pier Carteri 2010-05-30 14:57:54 UTC
New version of the code 

* Add zoom also for image preview
* Better handling of tags 
* Controls (play/pause button slice zoom buttons) are always visible but set sensivite only when needed
* Bugfix: Clicking somewhere in the slider while the clip is playing now yields a correct result

Git branch
http://github.com/m3tr0/pitivi---preview-on-filechooser
Comment 12 Jean-François Fortin Tam 2010-06-18 01:16:20 UTC
Ok, tested. Some more user-visible bugs:

- If you zoom in (2-3x maybe), when the playback slider reaches the end, it will go "off limits". Not only this looks weird, but it also causes error messages in the terminal, such as:

** (pitivi:23122): CRITICAL **: clearlooks_style_draw_box: assertion `width >= -1' failed

Also,
- There is no ":" between the tags and the tag values...
- Ideally, it would be good if the "zoom" level was remembered, at least for the current session (and perhaps forever).
- Major bug: if you play a clip and let it finish, then hit the play button again... all the tags are duplicated (and this happens +1 each time you do it).


Other than that, it's great and works as advertised (don't know if the handling of tags is perfect, but so far it's relatively satisfactory). Awesome!
Comment 13 Edward Hervey 2010-06-18 10:17:00 UTC
Jeff, ASSIGNED means you're going to be merging the work... which I doubt considering you don't have any pitivi.org git account.
Comment 14 Edward Hervey 2010-06-19 10:12:16 UTC
* It might be nice to have an option to disable previewing (yes, I know, but still).
* Make your classable also subclass from the log.loggable.Loggable class and add some debugging statements for easier debugging.
* I'm getting some weird backtraces on some files [1]
* If the discovery fails, it might be good to mention the file can't be use in pitivi (and maybe come up with a reason (like the error given by discoverer).

[1]:
Traceback (most recent call last):
  • File "pitivi/discoverer.py", line 445 in _busMessageEosCb
    self._finishAnalysis("EOS")
  • File "pitivi/discoverer.py", line 314 in _finishAnalysis
    if self._emitResult():
  • File "pitivi/discoverer.py", line 274 in _emitResult
    self._emitDone(factory)
  • File "pitivi/discoverer.py", line 234 in _emitDone
    self.emit("discovery-done", self.current_uri, factory)
  • File "pitivi/signalinterface.py", line 145 in emit
    *args, **kwargs)
  • File "pitivi/signalinterface.py", line 119 in emit
    res = cb(*ar, **kw)
  • File "pitivi/ui/filechooserpreview.py", line 141 in _update_preview
    self.show_preview(uri)
  • File "pitivi/ui/filechooserpreview.py", line 159 in show_preview
    pixbuf = gtk.gdk.pixbuf_new_from_file(gst.uri_get_location(uri))
glib.GError: Failed to load image '/data/videos/DSCN0729.MOV': Image has zero width
^
Comment 15 Edward Hervey 2010-06-19 10:16:57 UTC
I figured out why that backtrace appears. You assume that if one stream has the is_image flag set then you think it's an image.

You shouldn't do that, but instead check the type of the factory. If it's a PictureFileSourceFactory then it's an image, else it isn't.
Comment 16 Pier Carteri 2010-07-04 15:29:44 UTC
Uploaded a new version with the following differences:
-fix tags duplication
-fix in image detection (don't use is_image but  PictureFileSourceFactory)
-fix assertion `width >= -1' failed
-now PreviewWidget derives also from Loggable (add also debug statements)
-now PreviewWidget remembers zoom level 
-add option to enable/disable preview 
-better handling of preview errors

As usual git branch on:
http://github.com/m3tr0/pitivi---preview-on-filechooser
Comment 17 Jean-François Fortin Tam 2010-08-26 20:05:23 UTC
Tested It all seems good to me, except the "add option to enable/disable preview". This option is useless, it doesn't make sense not to have it activated by default. Could you remove it?
Comment 18 Edward Hervey 2010-08-27 13:37:36 UTC
If one is missing a decoder/demuxer to preview a file, what will happen ?
Comment 19 Jean-François Fortin Tam 2010-08-27 14:20:47 UTC
Created attachment 168904 [details]
screenshot when previewing fails

Then it will gracefully degrade, saying that it can't preview the file (same thing if you select a file that has nothing to do with previewing, such as .log file or whatever.
Comment 20 Pier Carteri 2010-08-29 17:41:27 UTC
about the option to enable or disable preview there is no problem for me to remove it from the code, and if you want it, please consider it already done, but I'm not so sure that is useless....
I agree it should be enable by default but, if the user simply does not want a fat filechooser widget with preview capabilities for whatever reason , than it should be possible to disable it.
Just my 1 cent
Comment 21 antistress 2010-08-29 17:50:14 UTC
I agree with  Jean-François Fortin Tam : if the computer is fast enough to run PiTiVi then running media preview should no be problematic.
I don't see any other reason to disable it.
Then having the option to enable or disable preview seems completely useless to me.
In the same way image previewer in GTK File Chooser has no "disable" option for instance
Comment 22 Jean-François Fortin Tam 2010-09-03 12:00:18 UTC
There are no downsides to having the previewer on because, from what I could test, your implementation degrades gracefully in case of failure.

There is a high cost in adding options that are not 100% necessary in the preferences, so I try to keep options to a bare minimum as part of the design philosophy. If you put an option there, either it is a really important choice for the user to care about, or it is something that has huge performance repercussions. If not, it is a failure to design for "what users want".

Further reference/examples:
http://mystilleef.blogspot.com/2005/12/just-add-option-to-turn-it-off-or-on.html
http://clarkbw.net/blog/2007/04/30/dont-do-what-i-want/
http://www.joelonsoftware.com/uibook/chapters/fog0000000059.html
http://www.codinghorror.com/blog/2006/09/unnecessary-dialogs-stopping-the-proceedings-with-idiocy.html
http://clarkbw.net/blog/2009/05/14/negotiate-with-your-users/
Comment 23 Pier Carteri 2010-09-03 12:16:04 UTC
Sorry for the delay...
My silence was not because I didn't want to remove the preference (you have convinced me :-) )but because my notebook end it's life on monday 30 august... I hope to have a new machine in this week-end. I will post the new version asap
Comment 24 Pier Carteri 2010-09-04 15:55:13 UTC
Uploaded a new version with the following differences:
- remove option to enable/disable preview. Preview is active by default

As usual git branch on:
http://github.com/m3tr0/pitivi---preview-on-filechooser

Sorry again for the delay....
Comment 25 Edward Hervey 2010-09-10 13:46:36 UTC
The code is definitely mergeable quality... but we don't have time for this release.

We'll be releasing again within a month, this should go in then.
Comment 26 Jean-François Fortin Tam 2010-11-30 19:24:19 UTC
As per last comment, can someone merge this before we get into the freeze again?
Comment 27 Brandon Lewis 2011-04-28 17:32:53 UTC
Created attachment 186834 [details]
Image displaying over-sized file chooser window
Comment 28 Brandon Lewis 2011-04-28 17:33:42 UTC
There needs to be some limit put on the size of the meta-data, because otherwise it causes the preview dialog to grow quite large for certain files.
Comment 29 Brandon Lewis 2011-04-28 17:36:35 UTC
Also get several of these warnings:

pitivi/ui/filechooserpreview.py:426: GtkWarning: Failed to set text from markup due to error parsing markup: Error on line 9: Entity did not end with a semicolon; most likely you used an ampersand character without intending to start an entity — escape ampersand as &amp;
Comment 30 Jean-François Fortin Tam 2011-05-22 17:29:20 UTC
This has now been merged in master, with some additional changes to polish it and fix some bugs. Thanks a lot, Pier!

(see also bug 586003 or bug 650807 if you want to take it a bit further)
Comment 31 Jean-François Fortin Tam 2011-05-22 17:30:32 UTC
Um, I meant bug #650806 and bug #650807.