search

getmango / Mango

Mango is a self-hosted manga server and web reader
https://getmango.app
MIT License
1.71k stars 123 forks source link

[Feature Request] Home screen (easy access to entries you're reading and recently added) #37

Closed jaredlt closed 4 years ago

jaredlt commented 4 years ago

Background

I really like plex.tv. I use it heavily for TV and Movies and think they've nailed the user experience. I discovered Mango as I was looking for a Plex for Comics/Manga. One of the many things I like about Plex is their home screen:

plex

They have:

Both Continue watching and On deck are fantastic for jumping back in where you left off, without having to remember where you were or what you were last watching. And Recently added is great for discoverability, especially for other users of the server.

Proposal

I would like to propose introducing this into Mango. Of course, this needs to be something that you think would benefit your vision for Mango, I don't want to step on your toes or take things in a direction you don't like :)

How would it work?

These are just my suggestions so far. All of it will need your feedback.

I have created a proof of concept for some of this:

mango-home-poc

Some notes:

Your thoughts

I have the code in my fork but it is HORRIBLE :) I really wanted to propose this without delving too deep to see if it's a direction you like and if it's something we should explore further together. Let me know what you think.

Update: adding todos here for current work plus things we might do later so we don't forget about them

Later / other

hkalexling commented 4 years ago

This looks amazing! Thanks for taking the time to write this :D

I am a heavy user of Plex as well, and I agree we need a more refined home page. If you want, I can invite you as a collaborator and create a branch, so we can both work on it and experiment with it.

Regarding the details:

You proposed a few different new features (all inspired by Plex)

  1. The new home tab with the "Continue Watching" and "On Deck (Up Next)" sections
  2. Plex theme
  3. New progress indicators (read page count and progress bar)

Maybe we should pick one and work on it first? I would vote for 1, but let me know what you think!

jaredlt commented 4 years ago

@hkalexling sounds great!

Agreed, let's focus on 1. I like Up Next too 👍

jaredlt commented 4 years ago

@hkalexling I started documenting the below requirements but now I'm wondering if we need both Continue Reading AND On Deck/Up Next.

Plex's logic displays in progress episodes in On Deck/Up Next too. I guess this may be a reason why Plex named it On Deck instead of Up Next. It represents all the shows you've started watching, including any episodes in progress. I think that makes sense. Continue watching is important for Plex because you can have multiple libraries eg. 1) TV 2) Movies. Movies will never be On Deck so it makes sense to highlight them in Continue Watching, and it would be confusing if this didn't also display TV Shows that are in progress too. If you go into the TV library dashboard of Plex it shows On Deck at the top (it has continue watching much further down and this displays all episodes that are in progress, even if from the same series).

If we just have a single library, could we have a single section called Continue Reading but it actually works like Plex's On Deck? It would show the next entry (or current entry if in progress). I'm leaning towards just one section now but let me know what you think.

I'll leave my original requirements below as is for your feedback. Either way we'll need to implement the On Deck functionality and after that it will be easy if we decide want to add the Continue Watching section too.

Continue Watching/Reading

Scenarios

{
  "comment": "Generated by Mango. DO NOT EDIT!",
  "progress": {
    "admin": {
      "Ghost in the Shell v1 (1995) (Uncensored)": 186,
      "Ghost in the Shell v2 (2010) (Digital) (danke-Empire)": 0
    }
  },
  "last_read": { // new section (need to consider methods to refactor eg. save_progress)
    "admin": {
      "Ghost in the Shell v1 (1995) (Uncensored)": "2020-05-18 12:12:36",
    }
  },
  //...
}

NB. In the future it may be worth exploring consolidating all of the info.json data in the database. This would allow you to add a user_entries table to avoid duplicating usernames in the json. It would also avoid modifying the user's library folder, which some users may not like, and could make some of these new features easier to deal with, especially for large libraries. This would change the source of truth from the file system to the db and there is much more to think about here. Out of scope for this feature :)

On Deck/Up Next

Definitions

Scenarios

hkalexling commented 4 years ago

I agree that the two sections can be merged. I think we need multi-library support later (see #13 ), but for now, merging the two makes more sense.

I try to avoid saving things to DB (unless absolutely necessary) due to the following reasons:

  1. If we store the title and entry information (progress, display names, cover images, etc.) in DB, for example, how can we refer to a certain title in DB? The only way I can think of is to use the title's path (e.g., library/manga_A ). However, if the title folder is moved or renamed, we won't be able to update the path in DB and the information will be lost. This won't be a problem with the current info.json setup, as the title folder still contains the same JSON file after moving/renaming.
  2. Mango is still in early development, and we might need to modify the DB scheme a lot if we are storing everything in DB. A simple JSON file is much more flexible.

It's a good point that users might not want the library folder to be modified. We need to plan carefully.

The rules you listed for displaying the sections are well-thought-out. I think we can go with that. Thanks for your efforts!

jaredlt commented 4 years ago

@hkalexling Proceeding with a single merged section for now 👍

RE: db vs file system - what you say makes perfect sense. And as you say, still in early development. Plex seem to handle moved files really well, by showing in the UI if they're missing and then knowing when they've come back, even if it's not the same file, but provided the naming and file structure is sufficient. I'll maybe have a poke around the Plex database to see if I can understand more how they do it, but yes, that is for much later on :)

I've pushed a new WIP branch up: feature/home. It includes:

Comments

hkalexling commented 4 years ago

Thanks for the update!

I refactored the get_on_deck_entry method and the code for the home route, and it should be cleaner now.

I tried using as much existing code as possible and calling e.book.load_percentage(username, e) but this always returned 0.0

In the home route method, I tried 

percentage = on_deck_entries.map do |e|
    e.book.load_percentage username, e.title
end

and it returns the percentage correctly.

Could I add this method into the Entry class? Didn't feel very DRY to do this though.

If we will be using the method a lot, feel free to add it, but I would suggest naming it Entry.load_percentage for consistency.

so it would be good if we could just call e.load_progress in the view without having to pass in the username variable.

We need the env variable in route methods to know who the logged-in user is. I can't think of a way to do it in the Entry class. Maybe I missed something? Also, could you elaborate a bit on the UserEntry class you mentioned? Why do we need it?

hkalexling commented 4 years ago

Also, the new section is titled "On Deck", but I think you mentioned that the section would be called "Continue Reading" but functions as the "On Deck" sections in Plex? I think "Continue Reading" is easier to understand, and it means pretty much the same thing in our case. Anyway, let me know what you think!

hkalexling commented 4 years ago

I saw your comment about DRY'ing the frontend HTML code, and I absolutely agree. The frontend code is ugly and it's getting harder to maintain. I have been planing to do a complete rewrite using a modern JS framework like React or Vue, but I just couldn't find the time to do it.

jaredlt commented 4 years ago

Much cleaner! Thank you :)

I've added your percentage approach and it worked fine. I must have been doing something wrong.

Have now refactored all "On Deck" references to "Continue Reading" (this was just a holdover from the old code I wrote before we agreed to merge the two sections).

RE: Frontend framework. Time is always the enemy! I've toyed a tiny bit with Vue and seemed to get on with it better than React. But Javascript has never been my strong point and don't think I have enough experience to recommend or be that much help in this area :/

Further ideas / still todo

The basic version of the feature is largely done. I have some further thoughts and ideas below but I'd like your feedback on a) what you agree with, and b) what should we include in this issue vs spinning off as a separate issues/features (I guess it's to decide if you're happy to keep iterating on the home screen here or if you want to close off the work and then explore further in new issues).

jaredlt commented 4 years ago

RE: UserEntry class. I clearly haven't properly thought this through :) I have most experience with Rails apps and using Rails with Devise (for auth) you get a really nice helper method current_user. In your controllers you create the objects needed in the view, and if the object should be limited to the user you can initialise it with eg. titles = current_user.context.library.titles (pseudo code). This means in your views, all of the attributes and class methods you reach for are already limited to what is available to the user:

<%- titles.each do |t| -%>
  <p>Percentage: <%= t.load_percentage %></p> <!-- Don't have to pass the user for each method -->
<%- end -%>

In a way this is just magic / syntactic sugar, but something like this could perhaps help down the line when you're looking to bring in filtering and visibility of multi libraries per user etc. Obviously I'm breezing over a lot of the complexity here and am probably being a bit naive with things. It's probably something you'd want to reach to a shard to help you solve. Anyway, this is way off scope. I'll leave this as food for thought for some future time!

hkalexling commented 4 years ago

I prefer keeping the relevant discussions in this issue as it's all about the home screen. We can open separate issues when working on the Plex theme or new progress indicators, etc.

I am thinking maybe we should allow the users to jump to the corresponding title page from a "Continue Reading" entry. Maybe we can add a "Go to Manga" button in the pop-up modal box? Let me know what you think :-)

Onboarding for new users

You are right and I haven't thought about this. I like your mockup and I can try to implement it. I think we should do something similar on the library page when the library is empty, but let's focus on the home screen first!

Order by last read and Recently Added

Yes I think we should have these two features ready before publishing a new version featuring the new home screen, and yes we should add the new fields in info.json. The "Recently Added" section would be particularly useful when we have the MangaDex auto-update feature (see #24). With the extra info saved in the JSON file, we can also have more sorting options in the library and title pages.

Your example info.json looks good, and I would suggest adding a time zone field just in case the local time zone changes. See https://crystal-lang.org/api/0.34.0/Time.html

Configurable "Continue Reading" settings

I think this feature would be nice to have but not pressing. It would be great if you have the time to work on it, otherwise, we can simply hardcode a limit (say 8 or 12) for the number of continue reading entries.

Other possible sections for Home screen

Yeah the extra sections you listed would indeed require external data sources, and I don't think a "Start Reading" section is necessary. The "Continue Reading" section we have now kind of acts like that, and the user can always visit the library page to "start reading". But maybe I misunderstood what you meant and in the case please let me know!

jaredlt commented 4 years ago

Jump to title page

Yes, definitely agree. There are quite a few buttons in the modal already and I've mocked up a slightly modified approach below. Inspired by Plex (as always!). My reasoning:

Anyway, not beholden to any of this. Just an idea :)

continue-reading-modal

Onboarding for new users

I'm happy to have a first go at this if it will help? I'm sure you have plenty of other things going on too.

Order by last read and Recently Added

Sounds good, I will proceed with these two.

Can you talk a bit more about the time zone field? I was planning to save the times in utc. Do you mean if the server time changes or the end user's? What is the implication?

Configurable "Continue Reading" settings

👍 I'll hardcode it for now but will have a think about this in the background.

Other possible sections for Home screen

By "Start Reading" I mean it would only show you titles for which you have not started at all. You're right that it's really just a shortcut to going to your library and sorting by Progress. I guess as your library gets bigger it might be a nice thing as a way to remind you of titles you haven't started. It's just another avenue for discoverability but don't think it's very important for now.

hkalexling commented 4 years ago

I'm happy to have a first go at this if it will help? I'm sure you have plenty of other things going on too.

Of course! That would be great 👍

Can you talk a bit more about the time zone field? I was planning to save the times in utc. Do you mean if the server time changes or the end user's? What is the implication?

Ah I thought you will just store the local time which might cause problems when the server time zone changes. If we are saving the time in UTC, perhaps we can make it more explicit? E.g.,

2016-02-15 10:20:30 UTC

One of the reasons I prefer using info.json over SQLite is that the data is more accessible to the users. Seeing implicit UTC time might be a bit confusing for them.

hkalexling commented 4 years ago

About the link to go to the title page, your mockup looks awesome! You're a great UX designer :D

jaredlt commented 4 years ago 
2016-02-15 10:20:30 UTC

Gotcha! I think when you store the String of the Time object it should it include this automatically but I will confirm.

About the link to go to the title page, your mockup looks awesome! You're a great UX designer :D

Thank you! :)

Will proceed with all as discussed 👍

jaredlt commented 4 years ago

Hi @hkalexling sorry for the slow movement on my end. Have had some work stuff I've had to take care of.

I'm currently implementing the last_read property into the info.json file. I'm a bit stuck on the load_last_read method and wondering if you can help? The basic gist of what I'm trying to do is below.

https://github.com/hkalexling/Mango/blob/e99d7b8b29db5e112390ea34fd6106a501655382/src/library.cr#L331-L341

Very similar to your load_progress method. Essentially, return the value if it exists in the file. I'm getting hung up on the strictness of Crystal and can't get it to work! :/ It either doesn't like the fact that sometimes the var doesn't exist, or there's some conflict with the var expected to be Time when sometimes it's nil? Unlike your load_progress method I can't really set the var initially as a Time object. As a hack, I did try setting last_read = Time.utc + 1.year and then checking if it's in the future it would assume it doesn't exist in the file and return nil, but couldn't get that to work either. I've tried a bunch of different things but perhaps you can give me some quick guidance? :)

hkalexling commented 4 years ago

Sure! I made some comments in your commit.

jaredlt commented 4 years ago

When you have a sec would be great to get your feedback on my latest commit.

I have implemented the Order Continue Reading by last read feature. I still feel like I'm fighting Crystal more than I'm productively coding :D That is to say, I got the thing working but if you think there's a completely different approach that would be better please let me know.

NB. I'm saving Time.utc directly into the json and this stores using the format "2020-05-29T15:27:54Z" (the Z denotes UTC). Is that ok? Alternatively we could use to_s which would give us the explicit UTC, but we'd need to convert back when we read the file etc.

hkalexling commented 4 years ago

Sorry for the delay! I've made some comments there :)

hkalexling commented 4 years ago

Sorry, I forgot to comment on the time zone issue. Yes, it looks good. No need to add UTC explicitly. I think we can assume only power users would look into the info.json file, and they should know what the Z means :)

jaredlt commented 4 years ago

I'm thinking about the implementation for the date_added property (so we can show and order the Recently Added section). It's complicated by the fact that for existing users, entries will already exist. For new entries, simply setting the date during the scan and doing nothing if the date already exists is fine. However, how best to handle existing entries?

  1. We could do a one-time scan & update, so all entries without date_added would be set with Time.utc ie. now. This is easiest for us but it does mean entries will appear that were not recently added (probably not ideal).
  2. Alternatively, we could set all existing entries as eg. date_added: null and then handle this case in the code?
    • How do we know that the scan has run (successfully) and we don't need to run it again? Add something to the config? Feels annoying that we have to introduce a setting for a one-off job.
  3. Some other approach...?

I'm leaning towards option 2 but wanted to see what you think.

hkalexling commented 4 years ago

Yeah, I agree it's quite complicated with many edge cases to consider. I have been thinking about this the whole time but I couldn't come up with a reasonable solution that works in all situations :(

I have a proposal but it's completely different from what we have planned, so I would like to know what you think.

On Unix, the OS stores three timestamps for each file: atime, ctime and mtime. atime is the time the file was last read, ctime is the time that the file's attributes (e.g., permissions and owners) were last modified, and mtime is the time that the file's content was last changed. Unfortunately, the file creation time is only available on some file systems, but I think we can use ctime to "approximate" the creation time. With this we don't need to store the date_added data in info.json, and we don't need to bother with all the edge cases.

However, Crystal removed the API to access ctime for portability issues (see https://github.com/crystal-lang/crystal/pull/5584#issuecomment-357505474). There's an open issue proposing to add in cross-platform ctime and atime supports (https://github.com/crystal-lang/crystal/issues/8357), but it's currently stalled.

So my plan is to write a simple C binding that reads the ctime of a file and use that to approximate the file creation time. We don't have to support Windows (at least for now), so we don't need to care about portability. What do you think?

jaredlt commented 4 years ago

My main concern with using the file timestamps is that they are not necessarily a true proxy for when the file was added to Mango. You could have a file on your harddrive (not in Mango yet) from a couple of months ago. If you moved it in today, it's possible that it won't even show in the Recently Added section as there are too many newer entries in the list (assuming my understanding is correct).

I also don't know enough about the internal behaviour of ctime. I primarily use Windows (although I heavily use WSL which Mango works great in). I know in Windows when you download an old file the original timestamp remains. Is it possible for a similar scenario with ctime? Eg. you download a file originally released in 2010 and that ctime stays as 2010?

I am happy to persevere for a while with the more explicit date_added approach and see if I get anywhere. Other than the issue regarding handling existing entries that I mention, are there other concerns you have that I should think about as well?

hkalexling commented 4 years ago

The nice thing about ctime is that whenever the file is moved, its ctime resets to the current time. The same thing happens when downloading a file. Here's a quick experiemnt to illustrate:

alex_ling@do-sg:~$ touch a
alex_ling@do-sg:~$ stat a
  File: a
  Size: 0           Blocks: 0          IO Block: 4096   regular empty file
Device: fc01h/64513d    Inode: 260261      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/alex_ling)   Gid: ( 1000/alex_ling)
Access: 2020-06-03 16:51:04.660381291 +0000
Modify: 2020-06-03 16:51:04.660381291 +0000
Change: 2020-06-03 16:51:04.660381291 +0000
 Birth: -
alex_ling@do-sg:~$ mkdir b && mv a b
alex_ling@do-sg:~$ stat b/a
  File: b/a
  Size: 0           Blocks: 0          IO Block: 4096   regular empty file
Device: fc01h/64513d    Inode: 260261      Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/alex_ling)   Gid: ( 1000/alex_ling)
Access: 2020-06-03 16:51:04.660381291 +0000
Modify: 2020-06-03 16:51:04.660381291 +0000
Change: 2020-06-03 16:51:14.924516830 +0000
 Birth: -

Notice that the Change time of the file a changes when it's moved.

How do we know that the scan has run (successfully) and we don't need to run it again? Add something to the config? Feels annoying that we have to introduce a setting for a one-off job.

I am not sure I understand this. The scanning process runs periodically. And what do you think should be added to the config?

jaredlt commented 4 years ago

Ah, that's perfect than. And even if you move your library or rename your title folder it doesn't modify the file ctime (on the same host at least).

RE: scan - I meant the one-off scan & update job that would run on server start to set the date_added: null for all existing files. If we went with this approach we'd need to know that the one-off job had run successfully so it wouldn't run the next time the server was started (because we would want new files to have date_added: {current time} rather than continue to set NULL for everything. With your ctime approach this is unnecessary.

I guess if we really wanted we could read the ctime value and store this into date_added, after which Mango reads from the json file? After doing a bit of reading:

ctime gets updated when the file attributes are changed, like changing the owner, changing the permission or moving the file to an other filesystem

It's possible ctime could be modified and this would break user expectations. Or am I worrying too much and is this overkill? :)

hkalexling commented 4 years ago

Ah now I see what you meant. Yes, I was thinking the same thing - how to properly initialize the date_added field for all entries, and I couldn't find a satisfactory solution. The user might stop Mango when the initialization is in progress.

I don't like the idea of adding a new config option either. I suppose we can add a special table into the SQLite DB to store these "states". For example, we can set a key to 0 before the initialization, and set it to 1 when it's finished. Still, it sounds hacky. Also, the initialization might take a long time if the library contains many archives.

Even if we are saving ctime as date_added for all entries, we will face the above issues.

Yes, ctime will change when the file's metadata is updated, or when the file's content is modified, and I don't think people will edit their manga archives.

Now let's consider some common operations that would change the ctime:

jaredlt commented 4 years ago

I think it's possible to have the best of both worlds. What if the load_date_added method looks in info.json and if it it doesn't exist it reaches for the ctime value and then assigns it to date_added in info.json? Subsequent calls to load_date_added will then always grab from info.json avoiding any issues caused by owner or permission changes.

hkalexling commented 4 years ago

👍 I like the idea! I will fork the branch and try to create a helper function that returns the ctime of a file.

jaredlt commented 4 years ago

Pushed latest code.

Please let me have your feedback.

What I'm planning to look at next:

hkalexling commented 4 years ago

The code looks good and works perfectly! Good job on that :D

Group recently added entries and home page onboarding: Yes, these are definitely needed. Please go ahead!

Slider: I see your point, but I am a bit concerned about the fact that users will have to click a few times to see the entries at the end of each section, while in the current state they can just scroll down and browse through them. Perhaps enabling autoplay would improve the experience?

jaredlt commented 4 years ago

No comments to refactor? I need to celebrate, thank you :)

Group recently added entries and home page onboarding: I'll focus on these for now 👍

Slider: I'm not a big fan of autoplay. Although it does a good job to showcase the content and teach users that there is more content then is shown, I find it can be quite distracting. You may be trying to read a title and it keeps moving, you may be trying to click a link and it moves away, which can be frustrating. I think it's a better user experience if the user is in control of scrolling or clicking.

But I agree that there is a discoverability problem to solve for first time users. Once a user discovers that they can scroll (or click) to see more content horizontally they should understand that this concept works for all sections on the Home screen (it's just Continue Reading and Recently Added for now, but if we do end up adding other sections - and/or allowing the user to configure which sections show on Home - then they should know that each section is usable in the same way, by scrolling horizontally).

Regarding clicking, I would configure it so that when you click the 'next' icon it doesn't just show 1 new entry but jumps to show all new entries (ie. if 4 columns showing entries 1 to 4, clicking Next will show 5 to 8, not 2 to 5).

Once a user knows they can scroll horizontally, there is effectively no difference to scrolling vertically. I would argue the 'Next' icons are there primarily to aid discoverability, to show that more content is available. For users on mobile and tablet touch screen devices, the horizontal scrolling should be quite natural (and we have Netflix to thank for teaching the UX pattern to a lot of people). For Laptop users you can two-finger horizontally scroll using your trackpad but perhaps not all users know this. For desktops you can hold Shift + Mouse scroll for horizontal scrolling, but again perhaps not all users know this. Perhaps we could show some one time messaging to teach users about this?

Other changes I've been thinking about to show more content are:

Anyway, I'm getting away from the main focus here and I've ended up writing a bit of an essay :/

Perhaps I could work on the Slider separately as an experiment?

hkalexling commented 4 years ago

Good points! You've convinced me autoplay is a bad idea :) Personally, I don't like one-time messages/tutorials as they can be a bit intrusive, and they are not easy to implement right. As a visual cue to let users know there are more entries available, perhaps we can add a dotnav to the slider?

I agree we should make better use of the horizontal space and make the cards and text smaller. I've been using the default styles came with UIKit but we should be able to tweak it easily. I feel that a slider to change the item size is kind of an overkill at this stage, and again I guess it's not easy to implement.

Feel free to experiment with the slider, but it would be great if you could submit a PR to merge the current branch to dev after finishing the entry grouping and user onboarding features. This branch is quite far behind dev, so I would need some time to resolve the conflicts and make some extra editings to make the new features (e.g., base URL and OPDS) work on the new home page.

jaredlt commented 4 years ago

Thinking about it a bit more I agree regarding one-time messages. A good UI should inform without needing them.

I'll experiment separately with the Slider and dotnav and report back. But will focus on the PR after the last two things. For future reference, if I'm creating a new branch is it better if I branch from dev or master? For this branch I split from master, shall I merge current master into it before submitting the PR, will that help you? I need to remind myself to merge the original branch back in regularly, sorry I haven't done that on this one.

My todos

Later / separate issues

jaredlt commented 4 years ago

Hmmm, I seem to have broken my local version of Mango :(

Whenever I try to access the site it says Waiting for localhost... forever. The console looks normal:

jaredt:Mango (feature/home)$ make run
crystal run src/mango.cr --error-trace
[INFO]    2020/06/07 14:03:01 | Scanned 2 titles in 110.915ms

I have reverted to my last known good commit and still seeing the issue. Have you encountered anything like this before? I'll see if you have any advice otherwise my next move will be to clone the repo again.

hkalexling commented 4 years ago

Yes, please fork from the dev branch in the future. The master branch contains the source code for the latest stable release, and the dev branch contains the new features/bug fixes to be released in the next version. You can see the development guideline for more details.

Don't worry! I will handle the conflicts because I am more familiar with the new features :) You can work on the slider or update UI in a new branch.

Sorry I haven't seen the error message before. You mentioned that you are developing in WSL so I guess it's a WSL problem/misconfiguration. I googled around, and these issues (https://github.com/microsoft/WSL/issues/4340 https://github.com/microsoft/WSL/issues/4769) seem related. Hope that helps!

jaredlt commented 4 years ago

Fixed it! Something got stuck in an infinite loop (I think) and there was still a version of Crystal running even though my terminal made it look like it exited. Once I fully closed the terminal and restarted it was working again :/ (on/off really is good advice!)

jaredlt commented 4 years ago

Pushed for final feedback before PR - commits

Empty Library empty-library

New user (not read any entry yet) never-read

Existing user without any entries in Continue Reading and nothing in Recently Added (rare scenario but handling rather than showing a blank screen) no-continue-reading-no-recently-added

hkalexling commented 4 years ago

Wow, that was fast! I've left some comments. Thanks for your efforts 👍

hkalexling commented 4 years ago

v0.6.0 has been released. Thanks a lot for your help :D

jaredlt commented 4 years ago

@hkalexling I've just noticed that the Recently Added section works slightly different than I expected. I see you have refactored the method and I wonder if the change in behaviour was intentional?

My original thinking with the Recently Added section was for it to act as a timeline. In its dumbest form it would list the entries, in order, newest to oldest. The important thing from the user's perspective is that the entries and their order do not change. Their position in history is fixed, thus their position in the Recently Added list is fixed.

Now it is a common case that a bunch of chapters for the same title will be added at the same time (eg. a friend tells you about a new manga so you download all the chapters). Rather than have the Recently Added list be completely filled up with the single title, it makes sense to intelligently group these together. But, at least in the way I was originally thinking, it would still act as a timeline. So if you added 3 entries for Manga A last week and another entry today, it would show 2 items in the Recently Added list.

In its current form (v0.6.1), a title will only appear once in the list, so if 2 entries were added three weeks ago and a new entry was added today, the old 2 new entries item will disappear and a new 3 new entries item will appear at the start of the list. This approach isn't bad, per se, just different. Although, I would argue that it is slightly more ambiguous as to "what" is recently added. I think it may be slightly harder for the user to build a mental model of how the section works. Eg.

Original state (newest to oldest):

New state:

Looking at this, we know that 1 new Hajime no Ippo entry was added today, but for an end user, without knowledge of the inner workings, it may be unclear.

My preferred new state (historical timeline is immutable):

NB. this would actually go a step further than my previous implementation. The 'grouping' should only group for a unit of time (using a 'day' seems most reasonable), otherwise if you only add chapters for a single title it would still cause the same grouping issue. Eg.

>> 2 weeks pass, no new entries for any titles. Today, add 1 new entry for Hajime no Ippo:

Current new state:

Preferred new state:

What do you think? Am I just overcomplicating things? There's something about the mutable list that irks me but I can't tell if it's just because it wasn't what I originally planned.

hkalexling commented 4 years ago

Ah sorry about that! I didn't know that was intentional. I thought it was incorrectly implemented and that's why I refactored it.

Actually I don't have a strong opinion on this, and both options sound fine to me. I wish we had more contributors/core team members to share their views on these matters :( Do you know how the "recently added" section works in Plex? I can experiment with it and see how they handle this.

jaredlt commented 4 years ago

No problem!

Plex works basically as I have described. I unashamedly 'steal' from their UX unless I can find fault in it :) Unlike us, they have the benefit of a full-time team and I assume have done a bunch of research to make the UX super simple. It's always nice to be able to take advantage of their hard work!

The only thing I'm not sure about is their grouping date range. It's definitely less than a week as all the weekly release TV shows appear as separate entries, but I think it's more than a day as I feel like a series may complete over a few days but it still groups it together.

hkalexling commented 4 years ago

I see. I agree it's better to follow their design. I will refactor the method again to bring back the original behavior. I will also experiment with the date range to find a good default value.

BTW, is there a specific reason for you to use three months as the threshold for "recently added"? I find the time span a bit too long to call it "recent", and I prefer setting it to one month. What do you think?

jaredlt commented 4 years ago

Sounds great, thank you!

RE: 3 months. No, I think I plucked that from the air. 1 Month sounds good to me 👍

hkalexling commented 4 years ago

The grouping behavior has been updated in v0.7.0 to respect your original design. I have used 1 day as the time limit for grouping neighboring entries.