Willy-JL / F95Checker

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

Reminders #85

Closed ghost closed 10 months ago

ghost commented 11 months ago

Here is my draft for reminders #81.

I've tested extension in Chrome, Firefox and integrated browser. Works fine. I have only one question regarding extension code. This check. Can't understand what is the purpose of it, we can't add icons to the same elements more then once because we are in a loop, and each function call removes old icons. Maybe I'm missing something major but it looks like a mistake or just a redundant code. I removed it for now.

You might want to have a good look at GUI code, since I've changed quite a lot of things. I tried to use a popup window but it looked ugly and cluttered, so I've decided to draw on main screen. Instead of opening a popup, you just switch to different view on main screen to manage reminders. This also allowed me to use already existing botombar to implement filtering(searching) functionality.

Willy-JL commented 11 months ago

That check in the extension I relieve was because the icons were being added twice at some point, for some reason. I never managed to figure it out, it was just iterating twice over the same elements in some cases, and this seemed to fix it so I never looked more into it. Idk if it's still an issue today

I'll have a look at the rest of the code tomorrow

Willy-JL commented 11 months ago

had a quick cursory look for now, it looks mostly fine, havent looked at the gui code yet tho and havent had the time to try it yet. anything on your end that you think is missing? or just me checking it out and merging?

and yeah if it can be made into a popup i think id prefer that, i can try to implement that myself tomorrow

ghost commented 11 months ago

Popup is possible, of course, but I encountered 2 problems:

  1. Since I'm drawing reminders in a table, it should be possible to select multiple reminders at once. But code that is drawing a rectangle over selected rows does not work properly in a popup, instead popup is closing and opening rapidly, when rows gets selected. I don't have enough expirience with imgui to fix that, so, just a skill issue :)
  2. Code/Concept duplication. We already have a list of stuff and a search bar, switching to a different list and using the same search bar makes more sense, then opening a popup window and essentially duplicating same layout inside of it. And, because you can attach notes to reminders, I imagine that many people will use them as an alternative to 'native' bookmarks, so searching functionality is certainly a must have.

It's your call of course, but in my opinion drawing in main panel is better. People using this feature will have familiar layout + more room, and others will never see this alternative view anyway.

ghost commented 11 months ago

I think I'm finished here. Try it, if everything is fine, you can merge.

Willy-JL commented 11 months ago

That's because in imgui by default, selectable will close pop ups (idea is that you have a multiple choice popup, selectable were initially added only for pop ups), and since I'm using an abstraction over simple imgui pop ups to better manage their state, it wasn't closed with the abstraction knowing about it so it just reopens it a frame later. This selectable behavior can be changed with a flag passed to the function

FaceCrap commented 10 months ago

Been looking at the extension code... I see you named it bookmarks in the code. I suggest now we settled on 'Reminders' as UI text, to also name it as such in the extension code. This may sound nitpicky, but it could prevent screwups in the future if actual site bookmarks get to play a part in F95Checker.

Also, while this code is now under review, please also adapt it to handle site search results and tag searches.

See this F95 thread post.

Screenshots: image

image

ghost commented 10 months ago

Every mention of 'bookmarks' was renamed to 'reminders' in commit 1854707, after extension code was finished.

I'll take a look at the search and tags.

Can you provide some feedback on notes? I've added them to reminders but it wasn't a part of initial feature request. Currently you can add notes to reminders and edit them, they will show up on hover. image I thought it would be useful to explain why reminder was created. You don't have to add them of course, notes can be empty. image What do you think @FaceCrap, will this be useful or is it just bloat?

ghost commented 10 months ago

Icons now appear in search

FaceCrap commented 10 months ago

Can you provide some feedback on notes? I've added them to reminders but it wasn't a part of initial feature request. Currently you can add notes to reminders and edit them, they will show up on hover.

I thought it would be useful to explain why reminder was created. You don't have to add them of course, notes can be empty. image What do you think @FaceCrap, will this be useful or is it just bloat?

These notes, are they stored in the same place (within the reminders section of F95Checker I mean) as the F95checker notes on regular tracked games? If so, then it would indeed be very useful. It would save a lot of going back and forth to F95Checker to read up why I added it to the Reminders in the first place. In fact, It would be so useful that I would welcome it if notes made in F95Checker for regular tracked games would show on hover of the main icon. I take it that this 'Show-on-Hover' effect also occurs in the thread itself?

Suggestion: I see an empty note has the literal text <Empty note>, maybe instead use a different colored icon for reminders with notes/comments? Saves you from hovering to see if a game actually has notes,

Since I'm not able to install Python 3.10+ (still got too many utilities depending on 3.9x), do you have an executable version I can play with?

ghost commented 10 months ago

Thank you for your feedback, here is fresh build to answer all your questions: https://workupload.com/file/rfr6hem8E9R This build also includes some uncommited changes, based on your suggestions.

Don't forget to back up your database, just in case. And load new extension from the archive.

FaceCrap commented 10 months ago

Question, something that you may not have thought about. At this moment I have ~63 games in my list, and about half a dozen to be added, which I want to keep track of, but at this point have no intention of playing or even downloading. These would be perfect candidates for the Reminders section, but I don't see a way to 'convert' an already added game (with a specific label) to a Reminder. How do I do that?

As far as I can see now, they can only be added through the extension, but clicking the button seems to just add it as a regular game, not as a Reminder. I switched to Reminder mode thinking that would be required, but the game still gets added as a regular entry.

It also would be useful if you could add Reminders manually instead, like pasting the link into the bottom input.

FaceCrap commented 10 months ago

I think this situation needs to be avoided with the extension image

I do like it now also shows at the bottom of a thread page in links leading to similar games if that game is in your library. It may have done this already, can't honestly not remember.

ghost commented 10 months ago

Question, something that you may not have thought about. At this moment I have ~63 games in my list, and about half a dozen to be added, which I want to keep track of, but at this point have no intention of playing or even downloading. These would be perfect candidates for the Reminders section, but I don't see a way to 'convert' an already added game (with a specific label) to a Reminder. How do I do that?

Currently, manually deleting game and creating reminder is the only way. But I see how this can be useful to some people, I'll add this later.

but clicking the button seems to just add it as a regular game, not as a Reminder

If 'button' here means extension icon, this is intended behavior. To create reminder you should use a context menu.

image

It also would be useful if you could add Reminders manually instead, like pasting the link into the bottom input.

I'm a little bit more familliar with the code base now, so I'm planning to add some missing features, including this one. They are not critical and I initially wasn't sure if it was worth it, considering added complexity and some code duplication required to achieve some of them.

I think this situation needs to be avoided with the extension

I'm pretty sure it was fixed in your build. Reopening a browser should help. Nevermind, it's an annoying bug. Good catch.

FaceCrap commented 10 months ago

Currently, manually deleting game and creating reminder is the only way. But I see how this can be useful to some people, I'll add this later.

That would be great, coz I got too many labeled as "Maybe's" to go through that list one by one. Ctrl-Selecting them in F95Checker, like you can do with the base features, and then converting them to reminders would be so much simpler. You already have the route from reminder to game list.

but clicking the button seems to just add it as a regular game, not as a Reminder

If 'button' here means extension icon, this is intended behavior. To create reminder you should use a context menu.

Alright, never used the menu. Found it.

You might want to reserve some space for the inevitable scrollbar, or at least have a bit of space in between notes and the sidebar if there's no need for a scrollbar yet.

image

It also would be useful if you could add Reminders manually instead, like pasting the link into the bottom input.

I'm a little bit more familliar with the code base now, so I'm planning to add some missing features, including this one. They are not critical and I initially wasn't sure if it was worth it, considering added complexity and some code duplication required to achieve some of them.

Why duplication? long time ago I coded stuff, but couldn't that be solved by using if/else checks to see which feature the code needs to hande?

~I'm pretty sure it was fixed in your build. Reopening a browser should help.~ Nevermind, it's an annoying bug. Good catch.

I can pull the extension straight from your repo, so wouldn't need a full recompile of the tool itself once you've fixed that.

Some things I'm almost sure which will come up when this goes public,

Is the display of the game reminder going to stay like this? (as in the part between /threads/ and the thread-id?) or will it become more like the regular game list? (e.g. date added column, status column, developer column...) To be honest, before I got to see it in action I was kinda expecting the layout to be similar to the regular game list, but just with a separate set of entries. Just like with regular games, folks probably want to keep track of individual reminders (I would), if and how often one gets updated, if it even still get updates, who is the dev (so we can see if they have more than one in development simultaneously, something that often does not bode well for the progress of one or the other.

Something else which is definitely going to be asked when made public, is the ability to sort on the thread column. I can see folks just populating this section with a boatload of game (I personally already have 65+ games on my "Maybe" list. Finding an entry is going to be a chore if the order is alphabetically random.

What I also expect is going to get asked is a way to monitor Reminders for changes. No point in keeping a reminder if the game gets abandoned, Something that happens all too often as of late after the arrest of that Australian developer. Not saying that's the reason, but some just plain got scared.

ghost commented 10 months ago

Is the display of the game reminder going to stay like this? (as in the part between /threads/ and the thread-id?) or will it become more like the regular game list? (e.g. date added column, status column, developer column...) To be honest, before I got to see it in action I was kinda expecting the layout to be similar to the regular game list, but just with a separate set of entries.

This is what I mean by code duplication, doing that will create two separate lists with the same functionality. I wanted to avoid that because it doesn't make sense and I know that something like this will never be merged. But you are right, if this goes public in the current state, people will ask for more and more functionality, until reminders basically mirror main game list. So, after thinking about it from this angle, here is my new proposal:

  1. While working on this feature, I've significantly improved browser extension. It really shouldn't be a part of this pull request, so I will open new one, with reminders part of it excluded.
  2. I will also open a new PR to implement simplified reminders, and code in this PR will be scraped with PR itself closed later.

New 'simplified' reminders will work like archiving feature, you will be able to convert your game to a reminder and back. Games converted to reminders will have different icon, like they do now, and will be hidden from game list. They will still be updated like regular games and you will be able to toggle their visibility through a button or a menu checkbox.

This will greatly simplify the code by leveraging existing sorting and filtering functionality.

FaceCrap commented 10 months ago

I must say, I think that's a smart move, if it accomplishes the same thing it would prevent a lot of headaches from getting swamped with requests to make it more like the regular list.

However, and speaking of old experience, why would extending your original method require duplicating code? I could be thinking too simplistic here, but would adding switches to the existing code to test whether it deals with a regular game or a reminder not prevent said 'need' for duplicating? I realize with your current approach this is now a moot point, just being curious here.

And if they're basically just like normal games, only with a different status analogue to archived games, will the tooltip on the icons then still show the notes like it did in your original reminder version? Going back to the original F95Checker with your extension in place, the icon now just shows it's present in the library but without note text

Ah, I see you took out this part... no problem, easily fixed. Putting that back in my local copy. Hmmm, apparently this requires something in the exe to be present, Looks like the original version doesn't retrieve notes...

I don't suppose you've got a compiled test version?

ghost commented 10 months ago

However, and speaking of old experience, why would extending your original method require duplicating code? I could be thinking too simplistic here, but would adding switches to the existing code to test whether it deals with a regular game or a reminder not prevent said 'need' for duplicating?

It's true, you can add a couple of if statements here and there to get it done, but it's still a lot of code. The alternative is extracting some bits of code that do the same thing into reusable functions, but that involves modifying 'critical' parts of the program, which will inevitably delay review process. I'm just trying to make my changes less intrusive and isolated so to speak so we can get code approved faster.

And if they're basically just like normal games, only with a different status analogue to archived games, will the tooltip on the icons then still show the notes like it did in your original reminder version?

Sure, everything we discussed should stay the same.

Hmmm, apparently this requires something in the exe to be present, Looks like the original version doesn't retrieve notes...

Yes, original RPC can only send thread ids to the extension, notes are not included.

I don't suppose you've got a compiled test version?

Not yet, once #90 is merged I will start working on the new implementation, progress on it should be much faster. I will include test version in the PR.

FaceCrap commented 10 months ago

I fiddled a bit with the extension code to make the icon on thread page titles the same height as the prefixes (e.g. VN, Ren'Py) and same alignement. This only affects the thread title and the links shown at the bottom. Didn't want to make a repo just for this so attaching the javascript zipper here. extension.zip Tested in Opera GX and Microsoft Edge. It's not perfect, but it's the best I could do with my limited CSS knowledge. The ones visible with bottom links aren't exactly the same height as the prefixes, but like I said, not perfect. Feel free to modify it further if you want or just ignore it. (note, changed the display from inline-flex to just inline... worked just as good, so removed the gap attribute.

ghost commented 9 months ago

I fiddled a bit with the extension code to make the icon on thread page titles the same height as the prefixes (e.g. VN, Ren'Py) and same alignement. This only affects the thread title and the links shown at the bottom. Didn't want to make a repo just for this so attaching the javascript zipper here. extension.zip Tested in Opera GX and Microsoft Edge. It's not perfect, but it's the best I could do with my limited CSS knowledge. The ones visible with bottom links aren't exactly the same height as the prefixes, but like I said, not perfect. Feel free to modify it further if you want or just ignore it. (note, changed the display from inline-flex to just inline... worked just as good, so removed the gap attribute.

I've checked out your changes, unfortunately moving all styling to container div breaks icon positioning on images (e.g Latest Updates page), and inline-flex is required to future proof extension, simple inline will stack icons on top of each other. I agree some adjustments are still necessary but currently there is no easy way to do them, changing styles to make some parts perfect will break other parts of the extension.

If you are still interested in reminders you can check out my fork, since Willy is apparently busy right now and progress on repo is slow, I've decided to maintain my own stuff (mostly for personal use). It's supposed to be more liberal, so if you have some crazy ideas feel free to open an issue.

FaceCrap commented 9 months ago

I fiddled a bit with the extension code to make the icon on thread page titles the same height as the prefixes (e.g. VN, Ren'Py) and same alignement. This only affects the thread title and the links shown at the bottom. Didn't want to make a repo just for this so attaching the javascript zipper here. extension.zip Tested in Opera GX and Microsoft Edge. It's not perfect, but it's the best I could do with my limited CSS knowledge. The ones visible with bottom links aren't exactly the same height as the prefixes, but like I said, not perfect. Feel free to modify it further if you want or just ignore it. (note, changed the display from inline-flex to just inline... worked just as good, so removed the gap attribute.

I've checked out your changes, unfortunately moving all styling to container div breaks icon positioning on images (e.g Latest Updates page), and inline-flex is required to future proof extension, simple inline will stack icons on top of each other. I agree some adjustments are still necessary but currently there is no easy way to do them, changing styles to make some parts perfect will break other parts of the extension.

If you are still interested in reminders you can check out my fork, since Willy is apparently busy right now and progress on repo is slow, I've decided to maintain my own stuff (mostly for personal use). It's supposed to be more liberal, so if you have some crazy ideas feel free to open an issue.

I am not sure where you see it breaking on images... I tested it on the Latest Updates page in both Edge and Opera, and in both the icon showed exactly where it should be. image

As for inline vs inline-flex, at this point that's a moot subject since we still don't have a release which shows more than one icon. Personally, I have not yet come across any page type where the icon didn't show as intended, but that may be just my used browsers and platform. How it looks on non-Windows platforms and other browsers is not that big a concern to me, but I understand it should be for public releases. Since my fiddling works on my platform and in my browsers, I'll keep the changes as is (but reverting to inline-flex whenever there's a release that is capable of showing more than one icon)

ghost commented 9 months ago

Personally, I have not yet come across any page type where the icon didn't show as intended, but that may be just my used browsers and platform.

My bad, I've been working on something in extension when I diffed your file and missed some code, it broke my version. I took a second look, and it looks good, I'll add these changes to next build of extended version.