Willy-JL / F95Checker

GNU General Public License v3.0
101 stars 16 forks source link

Timeline widget #138

Closed r37r05p3C7 closed 3 months ago

r37r05p3C7 commented 4 months ago

Simple timeline keeping track of different events. Here is a widget itself and all currently available events:

image

Full timestamp can be seen on hover(formatted using globals.settings.timestamp_format):

2024-02-25_14-57

I'll think about more possible events and promote to pr from draft later.

Also, let me know if you think TimelineEvent can be stored in a better way. I don't think sql_to_py can handle nested objects so I just used StrEnum and some pattern matching to work around that.

Willy-JL commented 4 months ago

I'm really liking this idea. Needs some work (for example launching could make a bit too many events), and I'll have a look at the code when I have the time, but deffo going in a good direction!

Willy-JL commented 4 months ago
Willy-JL commented 4 months ago

actually now i kinda wanna try doing these changes myself, sounds like fun lol

is there anything else you would we waiting on besides new events and what i said in order to undraft?

r37r05p3C7 commented 4 months ago

however, i dont see why a normal int enum would not work.

i did not use any IntEnum functionality such as additional properties in my impl, and strings are actually readable in db

storing the textual description makes it harder to update in the future if translations ever get considered or if the message needs to be changed. could just store the parameters as a tuple.

this is exactly what i was thinking about yesterday lol, partially why i kept it a draft. having a class for TimelineEvent would be ideal but i'm still confused a little bit about how exactly complex structures like classes are serialized/deserialized to/from database.

actually now i kinda wanna try doing these changes myself

great, like i said i'm still learning so i'll be happy to see a proper solution

is there anything else you would we waiting on besides new events and what i said in order to undraft?

not really, i think all important events are covered already, more can be added later

r37r05p3C7 commented 4 months ago

for example launching could make a bit too many events

yes, event pollution was one of my concerns as well, maybe some kind of filtering solution for frequent events can be introduced later when core functionality is implemented.

r37r05p3C7 commented 4 months ago

TimelineEvents is now a dataclass. Event types are stored in a separate IntEnumhack, along with message templates and icons. In the database, events are stored in a separate table, similar to labels and tabs. Events are loaded from the db once and "joined" with the games they are related to. Proper sorting is enforced on the initial load, by the database, using ORDER BY. During runtime, new events are inserted at the beginning of the list using insert(0, event). Drawing code was also adjusted, should be more efficient now.

FaceCrap commented 4 months ago

Looks like something useful. Will come in very handy to keep track of update cycles (as in how often does a game get updated). I think some kind of event filtering would also be an additional cool feature, things like when I launched or finished a particular update are of less importance to me. I'd be more interested in 'forum-side' events than those generated by local actions of myself. This could be done either on-the-fly, e.g. adding a dropdown with event types and checkboxes (much like you select which labels to apply to a game) and in the sidebar for a more default approach.

Since this will initialize only from the moment it becomes a live feature there's not going to be any historical data yet, so it might be useful to initialize the first display with the things we do know, (like when the game was added, the last time it was updated, the last time it was played, and the various known versions (installed, finished, latest update),

On comment though... I dislike wasted space... and the image in top shows a lot of that. Since you're forcing a sequential order already, the connection lines between the icons seems redundant. Losing that line would make the display a lot more compact.

r37r05p3C7 commented 4 months ago

I think some kind of event filtering would also be an additional cool feature This could be done either on-the-fly, e.g. adding a dropdown with event types and checkboxes

I think I'll place a dropdown within the "Timeline" tab itself, positioned right at the top for easy access. Do you think it's a good idea to persist timeline settings?

Since this will initialize only from the moment it becomes a live feature there's not going to be any historical data yet, so it might be useful to initialize the first display with the things we do know

GameAdded event already uses added_on field. It will be harder to retroactively add other fields like last_updated, last_played, etc. because they are not static. To include them in the timeline, we'd have to do a small database migration on startup, which I don't think is worth it.

On comment though... I dislike wasted space...

How about this layout? I prefer it because we won't have to deal with inconsistent, weird looking monospace fonts.

image

Willy-JL commented 4 months ago

I think I'll place a dropdown within the "Timeline" tab itself, positioned right at the top for easy access. Do you think it's a good idea to persist timeline settings?

yes and yes, imo

GameAdded event already uses added_on field. It will be harder to retroactively add other fields like last_updated, last_played, etc. because they are not static. To include them in the timeline, we'd have to do a small database migration on startup, which I don't think is worth it.

agreed

How about this layout? I prefer it because we won't have to deal with inconsistent, weird looking monospace fonts.

i like it

FaceCrap commented 4 months ago

Do you think it's a good idea to persist timeline settings?

Yes!. By all means Yes! That is why I suggested to also add the same dropdown in the sidebar since the sidebar seems the more logical place for persisting settings. Actually, I rather prefer to have the persisting option in the sidebar, so that on-the-fly changes don't persist. Otherwise, I have to remember to reset them to my preferred defaults before closing the More Info popup if I did change them there.

How about this layout? I prefer it because we won't have to deal with inconsistent, weird looking monospace fonts.

That wasts the same amount of space by just filling in the blanks through wrapping. Plus those 'chat bubbles' don't improve the look. If anything, monospace is as consistent as you can get with dates. I actually prefer a monospaced font when dealing with dates in columnar output. I even modified my local copy of the code to use the Meslo font for the actual date values because I very much dislike a proportional display when dealing with numbers and dates. No, what I meant was losing the connector line and circles around the icons and the blank line in between, a Timeline doesn't need to be literally visualized. The values themselves are explanatory enough to convey that.

That way the same data would use up about half the space as in below mock-up

2024-02-29_053344

Seeing as we already differ in opinion, why not add a toggle in the sidebar for Compact Timeline display ? This way you could have the display show as in your first screenshot, and if a user prefers a more compact display, use the form as in my mock-up if that setting gets toggled?

It will be harder to retroactively add other fields like last_updated, last_played, etc. because they are not static.

I don't mean to use these fields permanently, but when this goes live, there is no historical data present. So, when you open this tab the first time when this feature is added, the first entries for each game (Added, Last Updated, Last Played) can be filled by using the values existing at that moment. All you need to do is check if there's any data yet, if not then read those date values and any present version info for Last Updated and Last Played present at that time. After that new entries can just use whatever source you're using now.

FaceCrap commented 4 months ago

changed my comment slightly, seeing as there's new commits since I first posted. Might not have been seen before making those commits

r37r05p3C7 commented 4 months ago

Ok, I think I'm done making changes, it's officially ready to be reviewed.

I rather prefer to have the persisting option in the sidebar, so that on-the-fly changes don't persist

I decided to place Hide events button inside the tab because closing game popup to select events is annoying, and I don't think anyone will be making a lot of changes to their event preferences to warrant putting same picker in two places simultaneously.

That wasts the same amount of space by just filling in the blanks through wrapping. Plus those 'chat bubbles' don't improve the look.

I like them. I read through a shit ton of logs at work. I don't want to read more logs when using a game launcher, and your mockup looks a lot like a log file. But I understand everyone has their preferences, so there is an option in the sidebar to toggle compact timeline variant.

I don't mean to use these fields permanently, but when this goes live, there is no historical data present. So, when you open this tab the first time when this feature is added, the first entries for each game (Added, Last Updated, Last Played) can be filled by using the values existing at that moment.

Yes, and to do that we'll have to add populate_timeline_with_old_events() function and run it before UI is initialized. Not a big deal, but this little function will stay in the codebase forever because it's impossible to tell when all users will switch to new version.

Anyway, here is the final look:

Nomal Compact
image image

Full timestamp on hover is still there for both variants, and Hide event button opens a popup with checkboxes:

image

FaceCrap commented 4 months ago

Yes, and to do that we'll have to add populate_timeline_with_old_events() function and run it before UI is initialized. Not a big deal, but this little function will stay in the codebase forever because it's impossible to tell when all users will switch to new version.

Not sure if you meant this function is now present (haven't looked at the code yet) but I wouldn't call that an issue, there's more one-time-use code in the codebase, like when a user switches from a version still using the json format for game data, and almost every time a database change was made. That can't be avoided if updating from an old(er) version requires to convert data without losing existing data.

Still, thank you very much for considering my suggestions.

r37r05p3C7 commented 4 months ago

new events

image

r37r05p3C7 commented 4 months ago

btw @Willy-JL any progress on https://github.com/Willy-JL/F95Checker/issues/83? it would be nice to see the number of votes in the event message

Willy-JL commented 4 months ago

looking real nice, great work!

and no there is no progress on the number of votes, though it should be fairly trivial. just been busy with flipper stuff as the project i was leading just got hijacked away from me, so im transferring to a new project

FaceCrap commented 4 months ago

I hope this isn't too much to ask. I just thought about a possible new event candidate.

Have you considered adding an event for Last Full Refresh?

r37r05p3C7 commented 4 months ago

Have you considered adding an event for Last Full Refresh?

just to clarify, you mean firing an event every time when game is fully refreshed?

FaceCrap commented 4 months ago

Have you considered adding an event for Last Full Refresh?

just to clarify, you mean firing an event every time when game is fully refreshed?

Yes, for those games getting refreshed when the interval expires so to speak, or when a user forces a full refresh. Excluding Full Refreshes that occur when an update is published. I am not sure, but if I remember correctly, status and name changes are only detected when a full refresh is done. So it's not really necessary to also log an entry for a Full Refresh when this happens, but only those that happen because the interval expired. Or forced by the user. That would also make it clear which games or mods get refreshed every second day.

r37r05p3C7 commented 4 months ago

new events

image

also added icons in the filter menu

image

FaceCrap commented 4 months ago

Ran into a bug...

If you select one game and force a full refresh, no problem, but if you select more than one game, then it crashes on the second game. Seems like game is not passed to draw_game_recheck_button the second time around.

Traceback (most recent call last):
  File "Y:\Code\VN\F95Checker-TimeLineOrg\main.py", line 98, in <module>
    main()
  File "Y:\Code\VN\F95Checker-TimeLineOrg\main.py", line 50, in main
    globals.gui.main_loop()
  File "Y:\Code\VN\F95Checker-TimeLineOrg\modules\gui.py", line 930, in main_loop
    self.draw_games_list()
  File "Y:\Code\VN\F95Checker-TimeLineOrg\modules\gui.py", line 3161, in draw_games_list
    self.handle_game_hitbox_events(game, drag_drop=True)
  File "Y:\Code\VN\F95Checker-TimeLineOrg\modules\gui.py", line 2997, in handle_game_hitbox_events
    self.draw_game_context_menu()
  File "Y:\Code\VN\F95Checker-TimeLineOrg\modules\gui.py", line 1624, in draw_game_context_menu
    self.draw_game_recheck_button(game, f"{icons.reload_alert} Full Recheck", selectable=True)
  File "Y:\Code\VN\F95Checker-TimeLineOrg\modules\gui.py", line 1537, in draw_game_recheck_button
    game.add_timeline_event(TimelineEventType.RecheckUserReq)
    ^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'add_timeline_event'

Also got a crash due to a typing error (accidental drag?)

gui.py

1752                imgui.pop_style_olor()     #  (missing the c in color.)
1753                imgui.pop_font()
1754                if imgui.is_item_hovered():
1755                    with imgui.begin_tooltip():
1756                        imgcui.text(date.strftime(globals.settings.timestamp_format))  # (looks like the c from pop_style_color accidentally got dragged here : imgcui instead of imgui)
r37r05p3C7 commented 4 months ago

Seems like game is not passed to draw_game_recheck_button the second time around.

thanks, fixed

Also got a crash due to a typing error (accidental drag?)

i was really confused at first because i don't use a mouse when coding. it turns out you introduced this bug yourself locally.

FaceCrap commented 4 months ago

it turns out you introduced this bug yourself locally.

🤣oh man, I've no idea how, But can't deny the link. Honestly didn't think to do a second pull.

FaceCrap commented 3 months ago

@Willy-JL

image I was wondering already where the 16 days in @r37r05p3C7's screenshot, of the event getting logged, came from, but this one took the cake. 😂 At first, I thought he had rigged it, so both events got shown in the log for the screenshot, but I just saw the same thing happening.

It's happens because in

delta = dt.datetime.fromtimestamp(time.time()) - dt.datetime.fromtimestamp(game.last_full_check)

the dt.datetime.fromtimestamp(game.last_full_check) bit has the default epoch (January 1, 1970, 00:00:00 (UTC) value when adding a new game. This causes a full recheck to happen twice.

The first time because I forced it to pick up the correct game details since I just added the game, and the second time due to the expiration interval getting triggered. ...at least according to the timeline.

To prevent duplicate logging (it's purely cosmetic, as it doesn't have any ill side-effect), it might be an idea to initialize game.last_full_check also to time.time() when adding a new game, since a user is going to force a full recheck anyway after adding.

@r37r05p3C7 I am aware that you could do the same thing since it partly affects the timeline, but that would mean updating the PR again. And since it would only require one change, I thought as long as Willy is aware of it, he might already change this in the base code. So that whenever the timeline feature is merged, this 'cosmetic' issue won't affect the logging.

(I don't see doing a double full recheck as the main issue, since it's fast enough to not notice. after all, the refreshing itself isn't a timeline functionality, it just revealed it is happening.)

r37r05p3C7 commented 3 months ago

At first, I thought he had rigged it, so both events got shown in the log for the screenshot

That is exactly what I did. I never use Recheck button directly to pull info for new games(always Refresh), so I missed this obvious duplication "bug".

This causes a full recheck to happen twice.The first time because I forced it to pick up the correct game details since I just added the game, and the second time due to the expiration interval getting triggered.

To clarify, nothing was being rechecked twice, the problem was in my code, which logged both events at the same time if interval was expired and user requested a manual recheck.

it might be an idea to initialize game.last_full_check also to time.time() when adding a new game, since a user is going to force a full recheck anyway after adding.

I haven't checked, but it's safe to assume that many parts in the code rely on the fact that initial value is zero, so your proposed fix might require more work than you think.

I was wondering already where the 16 days in @r37r05p3C7's screenshot, of the event getting logged, came from

I tampered with the database values, but it could also happen "organically". You can avoid using Checker for a couple of weeks, then refresh it. If the dedicated api hasn't returned a new version, a full recheck will be forced, and the event log will reflect your two week absence.

FaceCrap commented 3 months ago

it might be an idea to initialize game.last_full_check also to time.time() when adding a new game, since a user is going to force a full recheck anyway after adding.

I haven't checked, but it's safe to assume that many parts in the code rely on the fact that initial value is zero, so your proposed fix might require more work than you think.

I am aware of this, and what I meant was, only when a new game is added. Just like having a newly added game marked as installed. Although the latter is optional. It can be left to its default epoch value in any other circumstance. But since we are talking about the date when a game was rechecked fully, I am pretty sure that every game has at least that field set already to a realistic date. So the only time it would be equal to the epoch is when a game is newly added and as long as it is not set by a user forcing one or through a periodic refresh.

Preventing it from getting logged twice is of course an additional safeguard.

FaceCrap commented 3 months ago

Just noticed that events don't get triggered on custom games. e.g. when you modify Version and Last Updated .

EDIT: Oops, that only applies to the things that can be changed via the More Info popup. Launches, setting it as finished and as installed DO get triggered.

EDIT2: While we're on the subject of Custom game, as the score now relies on votes to display the weighted value, it would be cool if we also could specify the # of votes. That is, if events for Custom games become a thing.

EDIT3: Ok, this is going to prove more difficult than anticipated. Database gets updated every g'damn keystroke. And each keystroke potentially generating a timeline event.

r37r05p3C7 commented 3 months ago

Database gets updated every g'damn keystroke.

yes, not going to happen

Willy-JL commented 3 months ago

Been a bit out of the loop, sorry. Is anything left for this or can it be merged?

r37r05p3C7 commented 3 months ago

it's done, ready to be merged