GNOME Bugzilla – Bug 755551
remove media specifiers as subclasses
Last modified: 2015-12-16 13:13:46 UTC
From IRC discussion: ... jasuarez | sometimes I think that having just a single GrlMedia() type would be enough jasuarez | and add "type" as another key toso | that what I do in lua-factory toso | to be generic enough jasuarez | we could do that in core, directly jasuarez | that would allow for new types of medias, like "playlists" toso | "games" ... hadess | the audio/video/photo stuff should probably just be helpers, there's really no point in them being actual subclasses jasuarez | yes, we make them subclasses because we though they would have higher abstract functions more suitable for each specific subclasses jasuarez | but right now they are data containers, so yes jasuarez | and would simplify api jasuarez | as right now some of them are duplicated, one for audio and another for video hadess | yeah
Created attachment 317436 [details] [review] core: merge key and relation functions Merge register_relation function into register_key, so when creating a new key make explicitly if the key is related with other.
Created attachment 317437 [details] [review] core: add GrlMedia media-type property This property will store the type of media (audio, video, image or container). The type for supported media in source has been renamed to GrlSupportedMedia.
Created attachment 317438 [details] [review] core: merge GrlMediaAudio into GrlMedia Get rid of GrlMediaAudio and use instead GrlMedia.
Created attachment 317439 [details] [review] core: merge GrlMediaVideo into GrlMedia Get rid of GrlMediaVideo and use instead GrlMedia.
Created attachment 317440 [details] [review] core: merge GrlMediaImage into GrlMedia Get rid of GrlMediaImage and use instead GrlMedia.
Created attachment 317441 [details] [review] core: merge GrlMediaBox into GrlMedia Get rid of GrlMediaBox and use instead GrlMedia. Also, all references to "box" are converted to "container", as container is a more suitable name than boxes.
Created attachment 317442 [details] [review] core: update serialize/unserialize functions Use the new flatten GrlMedia to serialize/unserialize. We keep the same format as in previous versions, except for the containers, which are now serialized as "grlcontainer" instead of "grlbox".
Created attachment 317443 [details] [review] all: use the new API to register keys The new API requires explicitly to specify the related keys.
Created attachment 317444 [details] [review] all: merge {audio,video,image,box} in GrlMedia All GrlMediaFoo types have been merged in GrlMedia.
Let's explain. The main work in these patches is getting rid of GrlMediaAudio/Video/Image/Box, and have only a single GrlMedia. The media still has a type (which is a property), and can be queried as "grl_media_is_audio/video/image_container()". Also, "Box" has been renamed as "Container", as most of the times we use the word Container to explain what the box does. So container seems a better name. The first patch in core and plugins is something unrelated, but I wrote it while working on the main feature. Basically, instead of defining keys and afterwards the relations in a separated manner, now we define the keys and set the related key explicitly. If the key is not related, then we use a NULL. This makes the developer to think more carefully about the relations when defining new custom keys, and not as something as they shouldn't care.
Review of attachment 317436 [details] [review]: Looks good ::: src/grl-registry.c @@ +552,3 @@ + if (bind_key == GRL_METADATA_KEY_INVALID) { + /* Key is only related to itself */ comment is not indent
Review of attachment 317437 [details] [review]: ::: src/grl-source.h @@ +127,3 @@ + GRL_SUPPORTED_MEDIA_ALL = (GRL_SUPPORTED_MEDIA_AUDIO | GRL_SUPPORTED_MEDIA_VIDEO | GRL_SUPPORTED_MEDIA_IMAGE) +} GrlSupportedMedia; + somewhere in the doc should say the version that this was introduced
Review of attachment 317438 [details] [review]: Sure
Review of attachment 317439 [details] [review]: As, for instance, grl_media_video_set_width is renamed to grl_media_set_width, the docs should point out that they were introduced in this next version. the same for _audio, _image, _container But that could be done in another commit, maybe even after the release.
Review of attachment 317440 [details] [review]: Yes
Review of attachment 317441 [details] [review]: Yep
Review of attachment 317442 [details] [review]: Looks good
Review of attachment 317443 [details] [review]: Looks good, test and dist works as well.
Review of attachment 317444 [details] [review]: Looks good, test and dist works as well. Played a little with grilo-test-ui with several sources, no problem found.
Created attachment 317482 [details] [review] doc: update documentation No more GrlMediaFoos.
(In reply to Victor Toso from comment #12) > Review of attachment 317437 [details] [review] [review]: > > ::: src/grl-source.h > @@ +127,3 @@ > + GRL_SUPPORTED_MEDIA_ALL = (GRL_SUPPORTED_MEDIA_AUDIO | > GRL_SUPPORTED_MEDIA_VIDEO | GRL_SUPPORTED_MEDIA_IMAGE) > +} GrlSupportedMedia; > + > > somewhere in the doc should say the version that this was introduced Yes. I usually add that information in the process release.
Created attachment 317487 [details] [review] doc: update documentation No more GrlMediaFoos.
Review of attachment 317487 [details] [review]: yep
Review of attachment 317436 [details] [review]: Sure.
Attachment 317436 [details] pushed as a9a7101 - core: merge key and relation functions Attachment 317437 [details] pushed as 900a3ab - core: add GrlMedia media-type property Attachment 317438 [details] pushed as 53e9029 - core: merge GrlMediaAudio into GrlMedia Attachment 317439 [details] pushed as 11c7c84 - core: merge GrlMediaVideo into GrlMedia Attachment 317440 [details] pushed as eb29c13 - core: merge GrlMediaImage into GrlMedia Attachment 317441 [details] pushed as 7241222 - core: merge GrlMediaBox into GrlMedia Attachment 317442 [details] pushed as 96cdec8 - core: update serialize/unserialize functions Attachment 317487 [details] pushed as 4760581 - doc: update documentation
Attachment 317443 [details] pushed as 6253377 - all: use the new API to register keys Attachment 317444 [details] pushed as d5e8910 - all: merge {audio,video,image,box} in GrlMedia