Opened 8 years ago

Last modified 8 years ago

#5365 in_progress enhancement

Some improvements to collections views

Reported by: Daniel Krol Owned by: Jessica Tallon
Priority: major Milestone:
Component: programming Keywords:
Cc: tsyesika Parent Tickets:

Description

My feature branch, which is split up into 3 commits at the moment:

https://github.com/orblivion/mediagoblin/commits/collection-ui-improvements

These are some improvements that I thought would be good, though it could be controversial. The sorting/filtering part is spawned from what I thought was somewhat of a requirement created by #5360, though independent enough that I split it out.

1) Media items in the collection view don't show the title of the media entry, we just show the collection item note. I think it makes sense to default to the media entry title if there is no note:

https://github.com/orblivion/mediagoblin/commit/25e424023226a4823b0f50258fe54a28964a202e

The potential issue I see with this is that collections can hold things other than media entries, and those might not have titles. I'm not sure what else you had in mind to hold, so I'll put this out there for now to see what you think. I could work around it but I want to make sure it's necessary.

2) Collections are currently sorted by descending position. I actually don't see anywhere in the codebase that position is ever set, so I presume this is a "TODO". The problem is that the way collections work now, it *sort of* sorts by ascending id. I guess the database does this naturally. The reason I say *sort of* is that I think some things start to get funny if you delete items. So, I thought I'd formalize it by sorting collections by ascending (position, id). If you start setting position and leave it as sorting descending, you'll end up reversing the way it's been working so far. (Unless you intended the sense of position and id to be opposite of each other?)

Also in this commit, I filter by whether the media item is processed.

https://github.com/orblivion/mediagoblin/commit/ad9a9d8a756a55e36e83696639d6dd558eb497e3

3) Collections currently show up as a text list. This seems rather unpolished. Here, I make it a gallery view, using the first item in the gallery if available:

https://github.com/orblivion/mediagoblin/commit/577911aed004e8a9fb2d2c6f9d133568dac3361c

Change History (7)

comment:1 by Daniel Krol, 8 years ago

Type: defectenhancement

comment:2 by Christopher Allan Webber, 8 years ago

Cc: tsyesika added

The first commit is super easy to merge. I'm down!

On the second commit, I think tsyesika needs to check sanity to see if this works well, since she knows the generic foreign key stuff and I don't :)

The thumb_url one, the third commit, is also kind of tied in with some of the changes tsyesika and I have been discussing anyway to expose the different image sizes. Unfortunately, doing it on a collection item is a bit messy in that this seems to be assuming images, and that's not always the case?

comment:3 by ShawnRisk, 8 years ago

Status: newreview

comment:4 by Jessica Tallon, 8 years ago

So, there are some problems now collections have become more generic. Collections no longer are just collections of MediaEnteries. Collections are now a more generic, unified model to store collections of any type, including multiple types in the same collection. These are some examples:

  • Inboxes of activities are collections
  • Comments are collections (often mixed type)
  • Collections of people (e.g. Followers, Following, Family, Friends, potentially block lists in the future).

This means we can't rely on collections containing things with a title. It also means we can't be sure they even have a thumb_url.

I love your suggestions however and we should definitely have a mechanism to produce the best representation of an object for all objects. This could be a thumb_url with a title, it could be a person's profile picture and name, it could it the name, date and contents of a text comment.

Thanks for making these patches, I might be able to merge the second one which refers to position as that should still be the same. I'll do my best to review it, as for the other tickets they will unfortunately have to be modified to work with more generic Collection models we have now.

comment:5 by Daniel Krol, 8 years ago

I did notice the fact that collections are not just for media items anymore, and I worked around it to the degree I could figure out before looking for feedback here. Having a preview for your collection seems to be a pretty standard feature for media galleries, so I hope we can find a way to make it work.

So, it sounds like my next task is to make this stuff more generic. I'm not entirely sure what to change. As for the first one, it checks whether the title exists before putting it. Why is that insufficient? Perhaps it will 500 if the object doesn't have an attribute called title (it's been a bit since I looked at this code so I don't remember). Do you want me to check for the existence of the attribute some other way? My personal opinion is that the interface to all this stuff should be simple in the end, otherwise it will be difficult to add these types of features. Another thing is, that particular view won't be showing things like comments, right? Or do you have no rules as to what types of things can go into the same collection as other types of things?

The second one you said may be fine. How about the third? Note that I filter by model_type to check whether the collection item is for a media entry before I grab its image URL, so it shouldn't try to grab one from, say, a comment. It will grab the first media entry object in the collection and take its thumb. If no media entry item is found, it goes with a default. What else should I do about it? Do you want me to put a thumb_url on all models that can go into a collection, and then not filter by model_type? As far as the change in the templates, yes it sort of assumes that it's a media item, but again, do you expect comments to be interspersed with my media items in a collection?

Thanks for checking out my contribution.

comment:6 by Jessica Tallon, 8 years ago

Owner: set to Jessica Tallon
Status: reviewin_progress

Okay, I apologize, I think I jumped to some assumptions with only a brief glance over the code. I'll do a proper review and see what, if anything does need to be changed. Sorry again for jumping to conclusions and thanks for making the contributions. I'll try and get these reviewed (and if possible merged) this week.

comment:7 by Daniel Krol, 8 years ago

No worries. My implementation may be kind of awkward, and may not catch all the cases you have in mind, so I'm looking forward to your feedback.

Note: See TracTickets for help on using tickets.