Add a "Calendar view" #732

justin-jiajia · 2025-01-10T01:58:24Z

Implement #338 .

muety

Thanks a lot for this, great contribution!

Regarding the overall implementation, I'd probably suggest a slightly different approach. You current implementation comes with a few drawbacks:

"Duplicate" logic to aggregate durations
Inefficient, because operating on raw heartbeats for each request instead of leveraging pre-aggregated summaries
Not respected custom mapping or project aliases

To overcome these (and thus also improve performance), I'd probably utilize the already existing summary computation logic. I'd (very roughly speaking) do approximately this:

Use SplitRangeByDays (search code for examples), to break down the requested interval into single days
Use SummaryService.Aliased() (just like in many other places throughout Wakapi) to fetch fetch summaries for each day
Construct the DailyProjectStat view model from an array of summaries, i.e. introduce methods func NewDailyProjectStats(summaries []*models.Summaries) []*DailyProjectStat { ... } or something, e.g. similar to here.

views/summary.tpl.html

models/summary.go

services/summary.go

routes/utils/summary_utils.go

muety

Thanks for implementing my change requests!

Do you think we should limit the calendar view component to a maximum number of days? Because when choosing a very large – if not All Time – interval, the chart becomes unreadable and super inefficient (blocked my browser tab for almost 30 seconds). Would love to hear your thoughts on that.

services/summary.go

…ata' for the summary webpage

justin-jiajia · 2025-01-17T11:29:12Z

Thank you for pointing out my problems in architecture. I've changed my codes to let them:

fetch 1 summary for each single day using FetchSummaryForDailyProjectStats(params *models.SummaryParams) ([]*models.Summary, error)
construct a DailyProjectViewModel from the summaries fetched below using NewDailyProjectStats(summaries []*models.Summary) []*DailyProjectViewModel

So: the "presentation layer", including the router & handler, present the data to the user directly without really calculating them, while the "service layer" calculates them and store them to the database.

I'm just a student and it's my first time to take part in the contribution of such a enormous project. Sorry for breaking the coding rules :-(

models/view/summary.go

muety · 2025-01-17T16:15:03Z

I'm just a student and it's my first time to take part in the contribution of such a enormous project. Sorry for breaking the coding rules :-(

No worries, dude! Getting into a new code base is hard, even more so with a maintainer who is quite opinionated about architecture and code style... I'm happy to guide you the way through Wakapi's code. Perhaps you'll even become keen to contribute on a regular basis once you're familiar with it.

muety

Found another two issue (maybe related):

It looks like either the date shown below the vertical bars is wrong (off by -1 day). If I choose "Today" (today is Jan 17th), the chart will show 2025/01/16. Similarly, if I choose "Yesterday", the chart will instead show 2025/01/15 (the day before yesterday).
The chart seems to omit the last day of the chosen interval. If I select "Past 7 Days", I'll end up with only six vertical bars, while the last one (today) is missing.

Also, can you please comment on my comment above regarding very large or very small intervals? I think the chart looks weird for less than three days or so and for more than 30 days (despite rendering the entire page unresponsive).

static/assets/js/summary.js

muety · 2025-01-17T16:09:30Z

routes/summary.go

@@ -41,6 +43,19 @@ func (h *SummaryHandler) RegisterRoutes(router chi.Router) {
 	router.Mount("/summary", r)
 }

+func (h *SummaryHandler) FetchSummaryForDailyProjectStats(params *models.SummaryParams) ([]*models.Summary, error) {


Minor one: make this a private method (lower-case) and move it below all public methods. Also, please stick to Go code style convention of having camel-case variables (e.g. cur_summary -> curSummary).

models/view/summary.go

justin-jiajia · 2025-01-22T12:10:00Z

Found another two issue (maybe related):

It looks like either the date shown below the vertical bars is wrong (off by -1 day). If I choose "Today" (today is Jan 17th), the chart will show 2025/01/16. Similarly, if I choose "Yesterday", the chart will instead show 2025/01/15 (the day before yesterday).

The chart seems to omit the last day of the chosen interval. If I select "Past 7 Days", I'll end up with only six vertical bars, while the last one (today) is missing.

Also, can you please comment on my comment above regarding very large or very small intervals? I think the chart looks weird for less than three days or so and for more than 30 days (despite rendering the entire page unresponsive).

I'm sorry but I'm unable to reproduce both of these problems under CST(UTC+8). Here's my screenshot for the 'past 7 days'.

Could you please provide me with your timezone & wakapiData.dailyStats in the source of the summary?

justin-jiajia · 2025-01-22T12:23:04Z

Thanks for implementing my change requests!

Do you think we should limit the calendar view component to a maximum number of days? Because when choosing a very large – if not All Time – interval, the chart becomes unreadable and super inefficient (blocked my browser tab for almost 30 seconds). Would love to hear your thoughts on that.

Yes. The problem is especially serious when the network to the database is slow. Or, in another word, server-side calculation also costs lots of time.

I've temporary limited the interval to (3days, 31days). However, if the user coded only 3 days in the past 3 month, the calendar view won't show up, either. This may lead to a worse user experience.

BTW, shall we us Time Cartesian Axis for the chart?

muety

Could you please provide me with your timezone & wakapiData.dailyStats in the source of the summary?

Never mind, I think it was an issue related to my test data. Tried again and seems to be working fine.

However, if the user coded only 3 days in the past 3 month, the calendar view won't show up, either.

Yes, I think that's not optimal. There should always be exactly as many vertical bars as there are days in the selected interval, otherwise users are going to be confused. To account for that, you could (a) either change your view model from [{date: '' project: '', duration: 0}] to a nested structure like [{ date: '', projects: [ { name: '', duration: 0 } ] }] to ensure one entry per day is present or, (b) fill missing dates front-end sided (probably a hassle) or, in the worst case (c) add a "dummy" project null (or so) to empty days.

BTW, shall we us Time Cartesian Axis for the chart?

Not very familiar with Chart.js, but sounds useful 👍

Btw., for whatever reason the activity chart isn't showing up anymore on your branch. Can you reproduce that and, if yes, fix it?

justin-jiajia · 2025-01-23T05:16:08Z

Could you please provide me with your timezone & wakapiData.dailyStats in the source of the summary?

Never mind, I think it was an issue related to my test data. Tried again and seems to be working fine.

I tried to fix another issue caused by time zones and this problem was resolved by the way. I didn’t realize it :)

However, if the user coded only 3 days in the past 3 month, the calendar view won't show up, either.

Yes, I think that's not optimal. There should always be exactly as many vertical bars as there are days in the selected interval, otherwise users are going to be confused. To account for that, you could (a) either change your view model from [{date: '' project: '', duration: 0}] to a nested structure like [{ date: '', projects: [ { name: '', duration: 0 } ] }] to ensure one entry per day is present or, (b) fill missing dates front-end sided (probably a hassle) or, in the worst case (c) add a "dummy" project null (or so) to empty days.

BTW, shall we us Time Cartesian Axis for the chart?

Not very familiar with Chart.js, but sounds useful 👍

Sorry for my vague expression. I wanted to say that Time Cartesian Axis does fix the missing vertical bars that were missing if the user didn’t code then. Here is an example.

Specifically, the axis make “Data spread according to the amount of time between data points”, providing a more simple approach of achieving the desired result. However, the time scale requires both a date library and a corresponding adapter to be present (see the link to the document above), which means that an extra JavaScript dependence will be added, drawing the size of the codebase.

Btw., for whatever reason the activity chart isn't showing up anymore on your branch. Can you reproduce that and, if yes, fix it?

Will try to fix if I can reproduce.

muety · 2025-01-23T06:15:45Z

I wanted to say that Time Cartesian Axis does fix the missing vertical bars

Perfect!

justin-jiajia · 2025-01-23T13:26:26Z

Considering the Time Cartesian Axis requires 2 extra JavaScript dependence, I implement the function using your (a) plan.

Btw., for whatever reason the activity chart isn't showing up anymore on your branch. Can you reproduce that and, if yes, fix it?

To test the function, some test data is required. However, my network connection to the production database server is extremely slow, so the svg chart isn't showing at any pages. So, BTW, is there a CLI commend to generate some fake data?

justin-jiajia · 2025-01-23T14:13:48Z

Forget to modify the rule of "no data". Will fix tomorrow.

muety · 2025-01-23T19:55:15Z

So, BTW, is there a CLI commend to generate some fake data?

You can use sample_data.py. Requires to install a bunch of Python dependencies though.

justin-jiajia · 2025-01-24T00:28:38Z

THX 获取Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Ferdinand Mütsch ***@***.***> Sent: Friday, January 24, 2025 3:55:38 AM To: muety/wakapi ***@***.***> Cc: Zijia Yan ***@***.***>; Author ***@***.***> Subject: Re: [muety/wakapi] Add a "Calendar view" (PR #732) So, BTW, is there a CLI commend to generate some fake data? You can use sample_data.py<https://github.com/muety/wakapi/blob/master/scripts/sample_data.py>. Requires to install a bunch of Python dependencies though. ― Reply to this email directly, view it on GitHub<#732 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AU6TG4UBXBAKH3H3HZT5FZL2MFCLVAVCNFSM6AAAAABU5MQC2KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJQHA4TEOJZGE>. You are receiving this because you authored the thread.Message ID: ***@***.***>

muety · 2025-01-26T16:02:42Z

Let me know when you're ready for another round of review ✌️.

justin-jiajia · 2025-01-27T13:24:56Z

The No data rule changed.

Btw., for whatever reason the activity chart isn't showing up anymore on your branch. Can you reproduce that and, if yes, fix it?

It was really weird. The data seemed to be stored to the activityChartSvg of the PetiteVue instance. However, for certain reason it was not rendered in the page. I tested even with ${ activityChartSvg } alongside the svg and it wasn't rendered either.

So I came up with testing the page with only ${ activityChartSvg } left on the page (the mount param were also deleted, with a v-scope added on the its parent). Surprisingly, it worked! So I committed them as a workaround. I'm not sure whether or not it's a bug of PetiteVue.

It's uncomfortable for me to look at so much space in the right of the page. Could you please fix it? (I'm not familiar with the grids system frontend)

muety

Your HTML was messed up, meaning your opening and closing divs didn't match. Please revert the last commit and instead apply the changes from the attached patch.

fix_activity_chart.patch

Nevertheless, things still don't look perfectly right. Besides there being no margin before your chart the logic still doesn't seem to be correct. See attached screencast.

Screencast.From.2025-01-31.10-23-37.mp4

justin-jiajia · 2025-01-31T10:27:19Z

Thanks for pointing out these problems, and I'll try to fix the margin and 'no data' notification a few days later after applying the patch, but I am unable to download the patch since opening the first link resulted in an error. Thanks again for your patient guidance.

…

________________________________ From: Ferdinand Mütsch ***@***.***> Sent: Friday, January 31, 2025 5:30:59 PM To: muety/wakapi ***@***.***> Cc: Zijia Yan ***@***.***>; Author ***@***.***> Subject: Re: [muety/wakapi] Add a "Calendar view" (PR #732) @muety requested changes on this pull request. Your HTML was messed up, meaning your opening and closing divs didn't match. Please revert the last commit and instead apply the changes from the attached patch. [fiy_activity_chart.patch]<https://camo.githubusercontent.com/1705b169f0c88888ceea4288a448da892f10ec150b6e3a26d0e135c6306f1d94/68747470733a2f2f70617374722e64652f702f766562766d636f356e6676637633346e6d7a6f713376696a> Nevertheless, things still don't look perfectly right. Besides there being no margin before your chart the logic still doesn't seem to be correct. See attached screencast. https://github.com/user-attachments/assets/e9dc2db2-0308-4bb8-b5cb-0eb0605f634b — Reply to this email directly, view it on GitHub<#732 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AU6TG4SDM2QZQBA5CKBSL232NM7FHAVCNFSM6AAAAABU5MQC2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOBWGEYDENZYGY>. You are receiving this because you authored the thread.Message ID: ***@***.***>

muety · 2025-01-31T10:37:01Z

Sorry, fixed the link!

justin-jiajia added 2 commits January 10, 2025 01:53

Add a "Calendar view" just like what's on the wakatime dashboard

5de8446

Merge branch 'master' of https://github.com/justin-jiajia/wakapi

2597d55

justin-jiajia marked this pull request as ready for review January 10, 2025 01:58

muety self-requested a review January 10, 2025 16:36

muety requested changes Jan 10, 2025

View reviewed changes

views/summary.tpl.html Outdated Show resolved Hide resolved

models/summary.go Outdated Show resolved Hide resolved

services/summary.go Outdated Show resolved Hide resolved

services/summary.go Outdated Show resolved Hide resolved

justin-jiajia added 2 commits January 11, 2025 20:03

Improve the porformance of the "Calendar view"

b03ba8a

Delete my nonsense of the mocks

015fa9f

justin-jiajia commented Jan 11, 2025

View reviewed changes

routes/utils/summary_utils.go Show resolved Hide resolved

justin-jiajia requested a review from muety January 14, 2025 10:00

muety requested changes Jan 15, 2025

View reviewed changes

services/summary.go Outdated Show resolved Hide resolved

services/summary.go Outdated Show resolved Hide resolved

Change the 'Calendar view' to meet the architecture rule & Show 'No D…

5d92d89

…ata' for the summary webpage

justin-jiajia requested a review from muety January 16, 2025 13:23

justin-jiajia commented Jan 17, 2025

View reviewed changes

models/view/summary.go Outdated Show resolved Hide resolved

muety requested changes Jan 17, 2025

View reviewed changes

Limit the interval of the 'Calendar View' & Fix typo

698a99d

muety self-requested a review January 22, 2025 21:05

muety requested changes Jan 22, 2025

View reviewed changes

Show all vertical bars for the 'Calendar View'

03a306c

justin-jiajia marked this pull request as draft January 23, 2025 14:12

justin-jiajia added 2 commits January 27, 2025 20:31

Fix the 'No data' for the Calendar view

776ae3f

Fix the activity chart

e4e3f37

justin-jiajia marked this pull request as ready for review January 27, 2025 13:05

justin-jiajia requested a review from muety January 27, 2025 13:05

muety requested changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a "Calendar view" #732

Add a "Calendar view" #732

justin-jiajia commented Jan 10, 2025

muety left a comment

muety left a comment •

edited

Loading

justin-jiajia commented Jan 17, 2025

muety commented Jan 17, 2025

muety left a comment •

edited

Loading

muety Jan 17, 2025

justin-jiajia commented Jan 22, 2025

justin-jiajia commented Jan 22, 2025

muety left a comment

justin-jiajia commented Jan 23, 2025 •

edited

Loading

muety commented Jan 23, 2025

justin-jiajia commented Jan 23, 2025

justin-jiajia commented Jan 23, 2025

muety commented Jan 23, 2025

justin-jiajia commented Jan 24, 2025 via email

muety commented Jan 26, 2025

justin-jiajia commented Jan 27, 2025 •

edited

Loading

muety left a comment •

edited

Loading

justin-jiajia commented Jan 31, 2025 via email

muety commented Jan 31, 2025

Add a "Calendar view" #732

Are you sure you want to change the base?

Add a "Calendar view" #732

Conversation

justin-jiajia commented Jan 10, 2025

muety left a comment

Choose a reason for hiding this comment

muety left a comment • edited Loading

Choose a reason for hiding this comment

justin-jiajia commented Jan 17, 2025

muety commented Jan 17, 2025

muety left a comment • edited Loading

Choose a reason for hiding this comment

muety Jan 17, 2025

Choose a reason for hiding this comment

justin-jiajia commented Jan 22, 2025

justin-jiajia commented Jan 22, 2025

muety left a comment

Choose a reason for hiding this comment

justin-jiajia commented Jan 23, 2025 • edited Loading

muety commented Jan 23, 2025

justin-jiajia commented Jan 23, 2025

justin-jiajia commented Jan 23, 2025

muety commented Jan 23, 2025

justin-jiajia commented Jan 24, 2025 via email

muety commented Jan 26, 2025

justin-jiajia commented Jan 27, 2025 • edited Loading

muety left a comment • edited Loading

Choose a reason for hiding this comment

justin-jiajia commented Jan 31, 2025 via email

muety commented Jan 31, 2025

muety left a comment •

edited

Loading

muety left a comment •

edited

Loading

justin-jiajia commented Jan 23, 2025 •

edited

Loading

justin-jiajia commented Jan 27, 2025 •

edited

Loading

muety left a comment •

edited

Loading