-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Statistics.koplugin: Calendar day with custom start time #10254
Conversation
That was fast :)
I wouldn't find that confusing. I would drop the 2nd commit and forget about complexifying that. (And if you don't, I want a setting to disable "day start at" in monthly view, because I may then end up sticking with day start at 00:00 just to avoid my monthly view to be messed up :) I also don't think we would really need minutes (ie 4h30 - anybody would be fine with 04:00 or 05:00 I guess), but as you wish. Pinging @jonnyl2 for more thoughts. |
Btw, a little odd thing I just noticed today is the ordering of the books at the top of the day timeline view: it is stable but felt random, I couldn't pinpoint the logic looking at a few days, only by looking at the code and confirming when looking at a lot more days.
I would expect either by longer reading time, amount of pages read, or by first page read time this day. |
Do I need to set some custom format to get these results? I tried English and French, they are all in the format of |
Oh right, I changed the duration format on my device some weeks ago, so I did not notice anything before. |
00a1c5e
to
c343dee
Compare
Made a few changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hour shift in the month view feels really odd to me, and would not be needed.
But if you intuitively felt it is needed to have the shift synced, others may feel the same way, so ok, the SQL ended up not too ugly.
plugins/statistics.koplugin/main.lua
Outdated
start_of_day_widget = DoubleSpinWidget:new{ | ||
left_text = C_("Time", "h"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
FROM ( | ||
SELECT start_time-? as start_time, duration, page_stat.id_book, book.title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Wondered a few minutes over this substraction start_time-offset
, but I think it's right.)
No noticable performance drop with this intermediate query ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WHERE and JOIN clauses are inside the intermediate query so theoretically the heavy lifting should only happen once (just like before). But I'm no expert of the inner workings of SQL. So I'm not really sure how the subtraction in the SELECT clause as well as the separation of GROUP BY and ORDER from the WHERE clause will affect the overall performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe someone with better knowledge someday can drop in here, but no noticeable performance drop now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keyword is SQLite ftr, not SQL. ;-)
You can always run the query with EXPLAIN
and see the impact.
Also here's a link I once saved: http://www.visophyte.org/blog/2010/04/06/performance-annotated-sqlite-explaination-visualizations-using-systemtap/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I could have been more accurate, SQLite it is :)
I tried adding explain
in front of the two commands with and without the modification, the results are only 2 and 3 more operations respectively. Does this count alone mean something or do I need to read into the details? @Frenzie (apologizing for not fully doing my research..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know SQLite that well either, which is indirectly what I was trying to say. 😄
That being said, even if this query were super slow (which I doubt it is; it looks simple enough and what you said about explain
seems to confirm that indeed it is) it probably still wouldn't really be noticeable.
There are some scenarios along these lines involving joins where SQLite might make something seemingly simple (for the likes of Postgres/MariaDB) into something much more complex and slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, using sqlite> explain query plan SELECT...
:
Before:
QUERY PLAN
|--SEARCH TABLE page_stat_data USING INDEX page_stat_data_start_time (start_time>? AND start_time<?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE numbers USING INTEGER PRIMARY KEY (rowid<?)
|--USE TEMP B-TREE FOR GROUP BY
`--USE TEMP B-TREE FOR ORDER BY
After (with starttime-0 harcoded - dunno how to use bind variables on the command line):
QUERY PLAN
|--SEARCH TABLE page_stat_data USING INDEX page_stat_data_start_time (start_time>? AND start_time<?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE numbers USING INTEGER PRIMARY KEY (rowid<?)
|--USE TEMP B-TREE FOR GROUP BY
`--USE TEMP B-TREE FOR ORDER BY
After (with startime-7200 harcoded):
QUERY PLAN
|--SEARCH TABLE page_stat_data USING INDEX page_stat_data_start_time (start_time>? AND start_time<?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE book USING INTEGER PRIMARY KEY (rowid=?)
|--SEARCH TABLE numbers USING INTEGER PRIMARY KEY (rowid<?)
|--USE TEMP B-TREE FOR GROUP BY
`--USE TEMP B-TREE FOR ORDER BY
So, they look all identical to me, so sqlite must optimize that (or do what it can with that :), and we can keep our readable version.
Also, I did not notice any slowdown when using it.
I'm thinking if we put the settings inside the daily timeline view, like a button on the top left, it would be very clear that this only affects the timeline. Then there's no need to change the calendar. What do you think? |
Like I did recently for Book map and Page browser I guess?
So, you're ready to give up on monthly view day start shift ?! :) It was just because of the wording of the setting ? Then, you can just name it "Timeline day start at 04:00" with separators above and below :) |
If the wording's not within the timeline view, then I'd still want the calendar days' black bars synchronized with the timeline :) Because I don't want surprises when I tap on a day with black bars say at the beginning but find nothing when the timeline opens. |
ce5d3f4
to
7906ed4
Compare
Tested on my Kobo, and there's something wrong: I didn't set any "day starts", so still the default of 00:00, untouched (no settings) , and I have many reading spans during the night (when I was sleeping :) (and blank afternoons) when I use swipe left/right , but not when going directly to a day from calendar view. (I don't have very regular reading periods - and bad memory :) - so it's hard for me to tell if all is good and the same as before. Except by doing screenshots with/without the patch, which is tedious, but I guess I'll do that if you ack and can fix the above issue.) |
Minor thought: should the horizontal line between 23:00 and 00:00 be thicker ? Or would that be distracting, and we can assume the users that starts his day at 04:00 won't care distinguishing reading before and after 00:00 ? |
I can't reproduce it. Can you show screenshots of the same day when entered directly and by swiping? |
I also can reproduce that on the emulator, so these screenshots are from it: (28 and 27 are after the DST change of 26, so probably nothing related to that.) 28, 27 and 26 when entered directly with tap on the squares: Then, tap on 28 then swipe to 27 > 26 > 27 > 28: Tell me if you need me to add |
There's a peculiar twelve hour shift backward when swiping. Possible cause: Maybe you can log them to see if they are 86400? |
With these logger.warn added: function CalendarDayView:onPrevPage()
if not self:prevPage() and self.day_ts - 82800 >= self.min_ts then
local current_day_ts = self.day_ts - (self.reader_statistics.settings.calendar_day_start_hour or 0) * 3600
- (self.reader_statistics.settings.calendar_day_start_minute or 0) * 60
local previous_day_ts = current_day_ts - 86400 + 10800 -- make sure it's the previous day
+ local logger = require("logger")
+ logger.warn("AAA self.day_ts", self.day_ts, "current_day_ts", current_day_ts, "previous_day_ts", previous_day_ts)
local previous_day_date = os.date("*t", previous_day_ts)
previous_day_ts = os.time({
year = previous_day_date.year,
month = previous_day_date.month,
day = previous_day_date.day,
})
local previous_day_length = current_day_ts - previous_day_ts
+ logger.warn("BBB self.day_ts", self.day_ts, "current_day_ts", current_day_ts, "previous_day_length", previous_day_length)
if self.day_ts - previous_day_length >= self.min_ts then
-- go to previous day
self.day_ts = self.day_ts - previous_day_length
+ logger.warn("CCC fixed self.day_ts", self.day_ts)
self:setupView()
end
end
return true
end |
My bad. I tested on my daytime computer without pulling the last commit. I am reproducing it now. |
Fixed by specifying the |
Seems fine now. |
Judging by the behavior, it's 12:00 by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Frenzie : fine with the English strings?
plugins/statistics.koplugin/main.lua
Outdated
min = self.settings.calendar_day_start_minute or 0, | ||
hour_max = 6, | ||
ok_text = _("Set time"), | ||
title_text = _("Set starting time of a day"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
Day starts at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Daily timeline starts at
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
plugins/statistics.koplugin/main.lua
Outdated
info_text =_([[ | ||
Set the time when the day timeline should start. | ||
|
||
If you read past midnight, and wish this reading to appear with the previous evening's reading, you can set this to 04:00 for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you read past midnight, and wish this reading to appear with the previous evening's reading, you can set this to 04:00 for example. | |
If you read past midnight, and wish this reading session to be considered part of the previous evening, use a value such as 04:00. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You misinterpreted that, or "reading session" is not clear, or confusing.
May be then:
If you read past midnight, and wish this reading to be displayed on the same screen with your previous evening reading, use a value such as 04:00.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably did, but "reading" feels ungrammatical in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, may be "reading session(s)" (it feels the second part needs plural):
If you read past midnight, and wish this reading session to be displayed on the same screen with your previous evening reading sessions, use a value such as 04:00.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you read past midnight, and would like this reading session to be displayed on the same screen with your previous evening reading sessions, use a value such as 04:00.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording updated :)
No. So long as it's at least 3 hours away from midnight (maybe 4 in case of DST?) it should be alright. Edit: This limit actually shouldn't matter since it's subtracted when going to another day.
I'm sure nobody would :) |
@weijiuqiao |
Something we didn't think about I guess: |
@mergen3107 You're welcome :)
Right. in |
(Had a quick look around, didn't dare touching it - and as I'm off soon, I won't make any PR). |
… custom offset (#10299) Continuing #10254 (comment)
KOReader 2023.04 "Solar Panel" ![koreader-2023-04-fs8](https://user-images.githubusercontent.com/202757/234975122-d7739c71-74aa-4cb0-a086-72a4bba70f52.png) It's been another busy month squashing many bugs. Our Mac users will be happy to hear that I told macOS we've supported HiDPI since long before anyone came up with such terminology (koreader#10341), and that the program can now natively build on M1 devices (koreader#10291). Solar panel credit: <https://openclipart.org/detail/294030/solar-energy> by gnokii We'd like to thank all contributors for their efforts. Some highlights since the previous release include: * Readerzooming: fix use of default settings (koreader#10205) @hius07 * ButtonDialog/ButtonDialogTitle: consistent 'width' handling (koreader#10230) @poire-z * MovableContainer: add support for anchoring initial position (koreader#10230) @poire-z * Book map, Page browser: add top left menu (koreader#10230) @poire-z * Book style tweak: add button with CSS suggestions (koreader#10230) @poire-z * crengine: fix parsing of multibytes encodings (koreader#10230) @poire-z * readerstyletweak: update profiles on unregistering in dispatcher (koreader#10247) @hius07 * Filesearcher: add search in book metadata (koreader#10198) @hius07 * Fix: Updated legacy directory, which crashed the program (koreader#10260) @Mochitto * ReaderLink: allow a forward location stack (koreader#10228) @yparitcher * BookInfo: add page information (koreader#10255) @hius07 * Center pdf manual zoom mode (koreader#10246) @nairyosangha * File browser: add Folder Menu (koreader#10275) @hius07 * Calendar view: add options to change start time of days (koreader#10254) @weijiuqiao * filechooser: fix crash on "unreadable content" (koreader#10283) @hius07 * Sync book statistics: add to dispatcher (koreader#10285) @ptrm * [plugin] Exporter: use util.getSafeFilename() to remove illegal characters from output filename (koreader#10282) @Mochitto * readerbookmark: fix writing pdf annotation (koreader#10287) @hius07 * Folder Menu: sign for Home folder (koreader#10288) @hius07 * ListMenu: show mark for books with highlights (koreader#10276) @hius07 * Calendar view's day view: thicker separator at 00:00 (koreader#10289) @poire-z * PageBrowser: tweak scrolling behaviour at book start/end (koreader#10289) @poire-z * Make `kodev check` feature complete (koreader#8682) @yparitcher * macOS: support for M1 building (koreader#10291) @ptrm * Reader: do not apply font size and spacing out of range (koreader#10295, koreader#10307) @hius07 * File browser: show Folder Menu on long-press on Home icon (koreader#10298) @hius07 * SSH.koplugin: fix cant stop SSH server bug when pid file's stale (koreader#10300) @weijiuqiao * PM: Optimize task queue handling around standby (koreader#10203) @zwim * statistic.koplugin: fix today's timeline showing next day when within custom offset (koreader#10299) @weijiuqiao * ReaderThumbnails: update cached page thumbnail on bookmark note change (koreader#10303) @hius07 * SDL: add multitouch support (koreader#10334) @Frenzie * SDL: add HiDPI support (koreader#10341) @Frenzie * BookInfo: fix crash on show cover (koreader#10315) @hius07 * Deal with table.pack corner-cases properly (koreader#10350) @NiLuJe * Android: add Tagus Gea support (<koreader/android-luajit-launcher#412>) @Alfedi [Full changelog](koreader/koreader@v2023.03...v2023.04) — [closed milestone issues](https://github.com/koreader/koreader/milestone/63?closed=1) --- Installation instructions: [Android](https://github.com/koreader/koreader/wiki/Installation-on-Android-devices) • [Cervantes](https://github.com/koreader/koreader/wiki/Installation-on-BQ-devices) • [ChromeOS](https://github.com/koreader/koreader/wiki/Installation-on-Chromebook-devices) • [Kindle](https://github.com/koreader/koreader/wiki/Installation-on-Kindle-devices) • [Kobo](https://github.com/koreader/koreader/wiki/Installation-on-Kobo-devices) • [PocketBook](https://github.com/koreader/koreader/wiki/Installation-on-PocketBook-devices) • [ReMarkable](https://github.com/koreader/koreader/wiki/Installation-on-ReMarkable) • [Desktop Linux](https://github.com/koreader/koreader/wiki/Installation-on-desktop-linux) • [MacOS](https://github.com/koreader/koreader/wiki/Installation-on-MacOS)
Closes #10250.
Made two commits, one is on the calendar day view and the other the calendar weeks view. The latter is quite ugly because in the sql commands, the offset is inserted in multiple places.
I tried another way with the sqls though, something like:
select *** from (select start_time - offset as start_time, *** from page_stat) as page_stat ***
so thestart_time
is first modified then compared but somehow it doesn't work.Still, I find it too confusing to just leave it if the two views are out of sync. Maybe there's a better way to do the sqls?
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)