Skip to content

Commit

Permalink
continue reading sorted by last read
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaredlt
jaredlt committed Jun 1, 2020
1 parent e99d7b8 commit 3ef6a7b
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 15 deletions.
22 changes: 17 additions & 5 deletions src/library.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,7 @@ class Title
else
info.progress[username][entry] = page
end
# should this be a separate method?
# eg. def save_last_read(username, entry)
# if so, we would need to open the json file twice every
# time we save. Does that matter?
# save last_read timestamp
if info.last_read[username]?.nil?
info.last_read[username] = {entry => Time.utc}
else
Expand Down Expand Up @@ -336,7 +333,6 @@ class Title
last_read = info.last_read[username][entry]
end
end
return nil if last_read.nil?
last_read
end

Expand All @@ -346,6 +342,12 @@ class Title
@entries[idx + 1]
end

def previous_entry(current_entry_obj)
idx = @entries.index current_entry_obj
return nil if idx.nil? || idx == 0
@entries[idx - 1]
end

def get_continue_reading_entry(username)
in_progress_entries = @entries.select do |e|
load_progress(username, e.title) > 0
Expand All @@ -360,6 +362,16 @@ class Title
latest_read_entry
end
end

# TODO: More concise title?

This comment has been minimized.

Sign in to view

Copy link
@hkalexling

hkalexling Jun 2, 2020

Member

Are we going to use this method somewhere else? If it's only used in the / route, perhaps we should write the logic in the route code instead of creating a new method here.

This comment has been minimized.

Sign in to view

Copy link
@jaredlt

jaredlt Jun 2, 2020

Author Collaborator

Good point, I'll move it. I guess the same could be said for the method get_continue_reading_entry too. Shall I move that as well? Also, in generally how do you feel about putting all the logic in the route? I could wrap most of it in a helper method and just call that from the route? I don't mind either way but if you did prefer to tidy it with a helper method where would you suggest I store the helper?

This comment has been minimized.

Sign in to view

Copy link
@hkalexling

hkalexling Jun 3, 2020

Member

Actually I have a better way to organize the code. Perhaps we can merge the get_last_read_for_continue_reading method into get_continue_reading_entry, so it directly returns a sorted array of continue reading entries? This way we won't introduce useless methods, and the router code will still be concise and readable.

I guess the same could be said for the method get_continue_reading_entry too.

Oh, don't worry about it. I have been working to add OPDS support, and the method is useful when constructing the landing page of the OPDS feed.

This comment has been minimized.

Sign in to view

Copy link
@jaredlt

jaredlt Jun 3, 2020

Author Collaborator

Is it ok for that entry to live within the Title class? Or should we move it to the Library class and call it get_continue_reading_entries?

This comment has been minimized.

Sign in to view

Copy link
@hkalexling

hkalexling Jun 3, 2020

Member

Or should we move it to the Library class and call it get_continue_reading_entries

Yes, that makes more sense. Good idea!
def get_last_read_for_continue_reading(username, entry_obj)
last_read = load_last_read username, entry_obj.title
if last_read.nil? # grab from previous entry if current entry hasn't been started yet
previous_entry = previous_entry(entry_obj)
return load_last_read username, previous_entry.title if previous_entry
end
last_read
end
end

class TitleInfo
Expand Down
37 changes: 34 additions & 3 deletions src/routes/main.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,41 @@ class MainRouter < Router
t.get_continue_reading_entry username
}.select Entry

percentage = continue_reading_entries.map do |e|
percentage = continue_reading_entries.map { |e|
e.book.load_percentage username, e.title
pp e.book.load_last_read username, e.title
end
}

last_read = continue_reading_entries.map { |e|
e.book.get_last_read_for_continue_reading username, e
}

# Group values in a NamedTuple for easier sorting
cr_entries = continue_reading_entries.map_with_index { |e, i|
{
entry: e,
percentage: percentage[i],
# if you're ok with the NamedTuple approach we could remove the
# percentage and last_read vars above and just call the methods
# here eg.
# perecentage: e.book.load_percentage username, e.title

This comment has been minimized.

Sign in to view

Copy link
@hkalexling

hkalexling Jun 2, 2020

Member

Yup, I like the NamedTuple approach 👍
last_read: last_read[i]
}
}
# I couldn't get the sort to work where last_read type is `Time | Nil`
# so I'm creating a new variable with just the entries that have last_read
# even still, I have to do another workaround within the sort below :/
cr_entries_not_nil = cr_entries.select { |e| e[:last_read] }
cr_entries_not_nil.sort! { |a, b|
# 'if' ensures values aren't nil otherwise the compiler errors
# because it still thinks the NamedTuple `last_read` can be nil
# even though we only 'select'ed the objects which have last_read
# there's probably a better way to do this
if (a_time = a[:last_read]) && (b_time = b[:last_read])
b_time <=> a_time
end
}
# add `last_read == nil` entries AFTER sorted entries
continue_reading = cr_entries_not_nil + cr_entries.select { |e| e[:last_read].nil? }

This comment has been minimized.

Sign in to view

Copy link
@hkalexling

hkalexling Jun 2, 2020 •
edited

Member

I will replace L98-L112 with

continue_reading.sort! do |a, b|
  next 0 if a[:last_read].nil? && b[:last_read].nil?
  next 1 if a[:last_read].nil?
  next -1 if b[:last_read].nil?
  a[:last_read].not_nil! <=> b[:last_read].not_nil!
end

It's effectively the same but more concise. Sometimes the Crystal compiler is not very smart, and you can use not_nil! if you are sure the value won't be nil.

This comment has been minimized.

Sign in to view

Copy link
@jaredlt

jaredlt Jun 2, 2020

Author Collaborator

Much more concise :) That not_nil! is good to know, thank you. Will definitely save me some lost time in the future!

layout "home"
rescue e
Expand Down
14 changes: 7 additions & 7 deletions src/views/home.ecr
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
<%- unless continue_reading_entries.empty? -%>
<h2 class="uk-title home-headings">Continue Reading</h2>
<div id="item-container-continue" class="uk-child-width-1-4@m uk-child-width-1-2" uk-grid>
<%- continue_reading_entries.each_with_index do |e, i| -%>
<div class="item" data-mtime="<%= e.mtime.to_unix %>" data-progress="<%= percentage[i] %>" id="<%= e.id %>">
<%- continue_reading.each do |cr| -%>
<div class="item" data-mtime="<%= cr[:entry].mtime.to_unix %>" data-progress="<%= cr[:percentage] %>" id="<%= cr[:entry].id %>">
<a class="acard">
<div class="uk-card uk-card-default" onclick="showModal(&quot;<%= e.encoded_path %>&quot;, '<%= e.pages %>', <%= (percentage[i] * 100).round(1) %>, &quot;<%= e.book.encoded_display_name %>&quot;, &quot;<%= e.encoded_display_name %>&quot;, '<%= e.title_id %>', '<%= e.id %>')">
<div class="uk-card uk-card-default" onclick="showModal(&quot;<%= cr[:entry].encoded_path %>&quot;, '<%= cr[:entry].pages %>', <%= (cr[:percentage] * 100).round(1) %>, &quot;<%= cr[:entry].book.encoded_display_name %>&quot;, &quot;<%= cr[:entry].encoded_display_name %>&quot;, '<%= cr[:entry].title_id %>', '<%= cr[:entry].id %>')">
<div class="uk-card-media-top">
<img data-src="<%= e.cover_url %>" alt="" data-width data-height uk-img>
<img data-src="<%= cr[:entry].cover_url %>" alt="" data-width data-height uk-img>
</div>
<div class="uk-card-body">
<div class="uk-card-badge uk-label"><%= (percentage[i] * 100).round(1) %>%</div>
<h3 class="uk-card-title break-word" data-title="<%= e.display_name.gsub("\"", "&quot;") %>"><%= e.display_name %></h3>
<p><%= e.pages %> pages</p>
<div class="uk-card-badge uk-label"><%= (cr[:percentage] * 100).round(1) %>%</div>
<h3 class="uk-card-title break-word" data-title="<%= cr[:entry].display_name.gsub("\"", "&quot;") %>"><%= cr[:entry].display_name %></h3>
<p><%= cr[:entry].pages %> pages</p>
</div>
</div>
</a>
Expand Down

0 comments on commit 3ef6a7b

Please sign in to comment.