Skip to content
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

Searching (WIP) #231

Merged
merged 8 commits into from Feb 14, 2016
Merged

Searching (WIP) #231

merged 8 commits into from Feb 14, 2016

Conversation

Mrmaxmeier
Copy link
Contributor

🚧 WIP

To-dos

  • ui and ux that doesn't suck
    • activity indicator
    • empty state
  • gc database data
    • after switching tabs
  • implement pagination
  • hide search behind env var ENABLE_SEARCH=1
  • battle the bitrot

@fasterthanlime
Copy link
Collaborator

@Mrmaxmeier I recently changed how the game-store passes data down to the app-store (since 0.11.0), so you might want to take a look at that first

@fasterthanlime
Copy link
Collaborator

lemme get the icomoon one for you

@fasterthanlime
Copy link
Collaborator

@Mrmaxmeier re throttling API calls, it's already handled in util/api.js, take a look near the end of the file, ctrl-f cooldown

@Mrmaxmeier
Copy link
Contributor Author

@fasterthanlime Can you take a look at the current state of this PR?

Pagination isn't ready yet, because the API doesn't return the total amount of search results.
I personally think the search bar doesn't quite fit in. The loading dimmer isn't really elegant either.
Let me know if there's something that I should change.

anim

@fasterthanlime
Copy link
Collaborator

Ah Max, the interaction looks neat! I agree that the UI needs some work
(and I wonder if the search bar shouldn't always be there?)

Anyway I'm currently focused on i18n+preferences with matias, but will take
a look at this soon

On Wed, Dec 23, 2015, 00:01 Mrmaxmeier notifications@github.com wrote:

@fasterthanlime https://github.com/fasterthanlime Can you take a look
at the current state of this PR?

Pagination isn't ready yet, because the API doesn't return the total
amount
https://github.com/itchio/itch/pull/231/files#diff-049047752b1fcc4ac913a85d04af3de9R170
of search results.
I personally think the search bar doesn't quite fit in. The loading dimmer
isn't really elegant either.
Let me know if there's something that I should change.

[image: anim]
https://cloud.githubusercontent.com/assets/3913977/11967710/61f93886-a905-11e5-9d5a-ec8d233f20f9.gif


Reply to this email directly or view it on GitHub
#231 (comment).

@fasterthanlime
Copy link
Collaborator

I've been thinking about this a lot lately and the main issue for me is that we can't cache all search results in the db - it'll get too big. We need a way to mark these game records as "transient" and be able to garbage collect them when we don't need them anymore.

I haven't yet found a reliable way to do that, there's a lot that can happen. For example let's say we had a _fetched_for field in our game db records, that contained a tab name such as "search/goblins".

Whenever we garbage collect the DB, we could see if the tab is still open and remove records whose tabs have been closed - but what if the game has been installed since? Or added to a collection? We need a way to check that too.

Maybe the DB garbage collection routine should first collect all game IDs from collections, owned, etc., so that it knows what not to delete - and then we don't even need _fetched_for

AppActions.search_fetched(query, game_ids, games)

if (fetched < total_items) {
await fetch_search_page(query, page + 1, games)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we probably shouldn't fetch all search pages in one go - as opposed to collections which we want to have available offline

@fasterthanlime
Copy link
Collaborator

I want to bring this PR and #328 into master so it doesn't bitrot again, but hide it behind environment variables until it's ready.

@Mrmaxmeier
Copy link
Contributor Author

Discarding all data that isn't visible to the user sounds like it's the best option considering backwards (and forwards) compatibility. Adding _fetched_for might be useful later though.

There are a few options for triggering the gc:

  • on startup
  • on shutdown
  • every x minutes of uptime
  • after x db operations

what do you think?
requestIdleCallback could be handy if it works in this situation.

@fasterthanlime
Copy link
Collaborator

requestIdleCallback could be handy if it works in this situation.

I didn't know about it! (docs page), sounds interesting

I was thinking of triggering the GC on tab closes & startup

@Mrmaxmeier Mrmaxmeier force-pushed the search-games branch 2 times, most recently from ab3fec0 to 81c5fd1 Compare January 30, 2016 12:34
@fasterthanlime fasterthanlime merged commit ad15b06 into itchio:master Feb 14, 2016
fasterthanlime added a commit that referenced this pull request Feb 14, 2016
@fasterthanlime
Copy link
Collaborator

@Mrmaxmeier finally merged this, already works quite well! Will make a few changes I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants