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

Improve tags suggestions #877

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion bookmarks/api/routes.py
Copy link
Owner

Choose a reason for hiding this comment

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

Can you maybe elaborate what you are trying to accomplish or what specific problem you are trying to solve? In general I'm a bit hesitant with proceeding with this change, as it adds a lot of complexity for very little value. Especially with duplicating the search logic on the server.

I can see the benefit of a "contains" search mode and there is an issue where people have requested this before. But does it really make a big difference if all tags are prefetched from the server once in a while? And do users really need to be able to configure the minimum amount of characters to trigger a search?

Apart from that this change would need to be covered by tests somehow, and the options should be part of the user profile rather than per instance. But let's figure out what should be part of this PR first before making further changes on it.

Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import logging

from django.core.exceptions import ImproperlyConfigured
from django.conf import settings

from rest_framework import viewsets, mixins, status
from rest_framework.decorators import action
from rest_framework.exceptions import ValidationError
from rest_framework.permissions import AllowAny
from rest_framework.response import Response
from rest_framework.routers import DefaultRouter
Expand Down Expand Up @@ -140,7 +144,30 @@ class TagViewSet(

def get_queryset(self):
user = self.request.user
return Tag.objects.all().filter(owner=user)
queryset = Tag.objects.filter(owner=user)

word = self.request.query_params.get("word", None)
if word:
min_length = getattr(settings, "LD_TAG_MINIMUM_CHARACTER", 1)
if len(word) < min_length:
raise ValidationError(
{"word": f"Word must be at least {min_length} characters long."}
)

match_type = getattr(settings, "LD_TAG_MATCH_TYPE", "starts_with")
if match_type == "contains":
queryset = queryset.filter(name__icontains=word)
elif match_type == "starts_with":
queryset = queryset.filter(name__istartswith=word)
else:
# Handle unexpected match_type values
raise ImproperlyConfigured(
{
"match_type": f"Invalid LD_TAG_MATCH_TYPE setting: '{match_type}'. "
"Expected 'contains' or 'starts_with'."
}
)
return queryset

def get_serializer_context(self):
return {"user": self.request.user}
Expand Down
7 changes: 5 additions & 2 deletions bookmarks/frontend/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ export class Api {
.then((data) => data.results);
}

getTags(options = { limit: 100, offset: 0 }) {
const url = `${this.baseUrl}tags/?limit=${options.limit}&offset=${options.offset}`;
getTags(options = { limit: 100, offset: 0, word: undefined }) {
let url = `${this.baseUrl}tags/?limit=${options.limit}&offset=${options.offset}`;
if (options.word) {
url += `&word=${options.word}`;
}

return fetch(url)
.then((response) => response.json())
Expand Down
2 changes: 2 additions & 0 deletions bookmarks/frontend/behaviors/search-autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class SearchAutocomplete extends Behavior {
shared: input.dataset.shared,
unread: input.dataset.unread,
},
fetchType: input.getAttribute("data-fetch-type") || "static",
matchType: input.getAttribute("data-match-type") || "starts_with",
},
});

Expand Down
2 changes: 2 additions & 0 deletions bookmarks/frontend/behaviors/tag-autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class TagAutocomplete extends Behavior {
value: input.value,
placeholder: input.getAttribute("placeholder") || "",
variant: input.getAttribute("variant"),
fetchType: container.getAttribute("data-fetch-type") || "static",
matchType: container.getAttribute("data-match-type") || "starts_with",
},
});

Expand Down
35 changes: 0 additions & 35 deletions bookmarks/frontend/cache.js

This file was deleted.

23 changes: 13 additions & 10 deletions bookmarks/frontend/components/SearchAutoComplete.svelte
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<script>
import {SearchHistory} from "./SearchHistory";
import {api} from "../api";
import {cache} from "../cache";
import {tags as cache} from "../tags";
import {clampText, debounce, getCurrentWord, getCurrentWordBounds} from "../util";

const searchHistory = new SearchHistory()
Expand All @@ -12,6 +12,8 @@
export let mode = '';
export let search;
export let linkTarget = '_blank';
export let fetchType = 'static';
export let matchType = 'starts_with';

let isFocus = false;
let isOpen = false;
Expand Down Expand Up @@ -88,19 +90,20 @@
}

// Tag suggestions
const tags = await cache.getTags();
let tagSuggestions = []
const currentWord = getCurrentWord(input)
if (currentWord && currentWord.length > 1 && currentWord[0] === '#') {
const searchTag = currentWord.substring(1, currentWord.length)
tagSuggestions = (tags || []).filter(tag => tag.name.toLowerCase().indexOf(searchTag.toLowerCase()) === 0)
.slice(0, 5)
.map(tag => ({
type: 'tag',
index: nextIndex(),
label: `#${tag.name}`,
tagName: tag.name
}))
tagSuggestions = (await cache.getTags(fetchType, matchType, searchTag))
.slice(0, 5)
.map((tag) => {
return {
type: 'tag',
index: nextIndex(),
label: `#${tag.name}`,
tagName: tag.name
}
})
}

// Recent search suggestions
Expand Down
20 changes: 13 additions & 7 deletions bookmarks/frontend/components/TagAutocomplete.svelte
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
<script>
import {cache} from "../cache";
import {getCurrentWord, getCurrentWordBounds} from "../util";
import {tags as cache} from "../tags";
import {debounce, getCurrentWord, getCurrentWordBounds} from "../util";

export let id;
export let name;
export let value;
export let placeholder;
export let variant = 'default';

export let fetchType = 'static';
export let matchType = 'starts_with';

let isFocus = false;
let isOpen = false;
let input = null;
Expand All @@ -28,12 +31,12 @@
async function handleInput(e) {
input = e.target;

const tags = await cache.getTags();
const word = getCurrentWord(input);
debouncedFetchTags(input);
}

suggestions = word
? tags.filter(tag => tag.name.toLowerCase().indexOf(word.toLowerCase()) === 0)
: [];
async function fetchTags(input) {
const word = getCurrentWord(input);
suggestions = await cache.getTags(fetchType, matchType, word);

if (word && suggestions.length > 0) {
open();
Expand All @@ -42,6 +45,8 @@
}
}

const debouncedFetchTags = debounce(fetchTags);

function handleKeyDown(e) {
if (isOpen && (e.keyCode === 13 || e.keyCode === 9)) {
const suggestion = suggestions[selectedIndex];
Expand Down Expand Up @@ -101,6 +106,7 @@
}
}, 0);
}

</script>

<div class="form-autocomplete" class:small={variant === 'small'}>
Expand Down
2 changes: 1 addition & 1 deletion bookmarks/frontend/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@ import "./behaviors/tag-modal";
export { default as TagAutoComplete } from "./components/TagAutocomplete.svelte";
export { default as SearchAutoComplete } from "./components/SearchAutoComplete.svelte";
export { api } from "./api";
export { cache } from "./cache";
export { tags } from "./tags";
124 changes: 124 additions & 0 deletions bookmarks/frontend/tags.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
import { api } from "./api";

class Tags {
api;
staticTable;

constructor(api) {
this.api = api;

// Reset cached tags after a form submission
document.addEventListener("turbo:submit-end", () => {
this.staticTable = null;
});
}

async getTags(
fetchType /* : 'static' | 'dynamic' */,
matchType /*: 'starts_with' | 'contains'*/,
word /*: string */,
) {
if (fetchType !== "static" && fetchType !== "dynamic") {
if (fetchType !== undefined) {
console.warn(`Invalid fetch type passed as fetch type:`, fetchType);
}

fetchType = "static";
}

if (matchType !== "starts_with" && matchType !== "contains") {
if (matchType !== undefined) {
console.warn(`Invalid match type passed as match type:`, matchType);
}

matchType = "starts_with";
}

switch (fetchType) {
case "static":
return this._getTypesWithStaticTable(matchType, word);

case "dynamic":
return this._getTypesWithDynamicTable(matchType, word);

default:
console.error(`unreachable`);
}
}

async _getTypesWithStaticTable(
matchType /*: 'starts_with' | 'contains'*/,
word /*: string */,
) {
if (!this.staticTable) {
this.staticTable = await this._getAllTags();
}

return this._matchTags(this.staticTable, matchType, word);
}

async _getTypesWithDynamicTable(
matchType /*: 'starts_with' | 'contains'*/,
word /*: string */,
) {
const table = await this._getSpecificTags(word);

return this._matchTags(table, matchType, word);
}

async _getAllTags() {
return this.api
.getTags({
offset: 0,
limit: 5000,
})
.catch((e) => {
console.error(`Tags: Error fetching tags:`, e);
return [];
});
}

async _getSpecificTags(word /*: string */) {
if (word) {
return this.api
.getTags({
offset: 0,
limit: 50,
word: word,
})
.catch((e) => {
console.error(`Tags: Error fetching specific ${word} tags:`, e);
return [];
});
} else {
return this.api
.getTags({
offset: 0,
limit: 50,
})
.catch((e) => {
console.error(`Tags: Error fetching specific ${word} tags:`, e);
return [];
});
}
}

_matchTags(
tags,
matchType /*: 'starts_with' | 'contains'*/,
word /*: string */,
) {
if (!Array.isArray(tags)) return [];

word = word.toLocaleLowerCase();

return tags.filter((tag) => {
const lower = tag.name.toLocaleLowerCase();
return matchType === "starts_with"
? lower.startsWith(word)
: lower.includes(word);
});
}
}

export const tags = new Tags(api);
2 changes: 1 addition & 1 deletion bookmarks/templates/bookmarks/form.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
The form has been pre-filled with the existing bookmark, and saving the form will update the existing bookmark.
</div>
</div>
<div class="form-group" ld-tag-autocomplete>
<div class="form-group" ld-tag-autocomplete data-match-type="{{ tag_match_type }}" data-fetch-type="{{ tag_fetch_type }}">
<label for="{{ form.tag_string.id_for_label }}" class="form-label">Tags</label>
{{ form.tag_string|add_class:"form-input"|attr:"autocomplete:off"|attr:"autocapitalize:off" }}
<div class="form-input-hint">
Expand Down
4 changes: 3 additions & 1 deletion bookmarks/templates/bookmarks/search.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
data-mode="{{ mode }}"
data-user="{{ search.user }}"
data-shared="{{ search.shared }}"
data-unread="{{ search.unread }}">
data-unread="{{ search.unread }}"
data-match-type="{{ tag_match_type }}"
data-fetch-type="{{ tag_fetch_type }}">
<input type="submit" value="Search" class="d-none">
{% for hidden_field in search_form.hidden_fields %}
{{ hidden_field }}
Expand Down
5 changes: 5 additions & 0 deletions bookmarks/templatetags/bookmarks.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import List

from django import template
from django.conf import settings

from bookmarks.models import (
BookmarkForm,
Expand All @@ -26,6 +27,8 @@ def bookmark_form(
"auto_close": auto_close,
"bookmark_id": bookmark_id,
"cancel_url": cancel_url,
"tag_match_type": settings.LD_TAG_MATCH_TYPE,
"tag_fetch_type": settings.LD_TAG_FETCH_TYPE,
}


Expand All @@ -47,6 +50,8 @@ def bookmark_search(context, search: BookmarkSearch, mode: str = ""):
"search_form": search_form,
"preferences_form": preferences_form,
"mode": mode,
"tag_match_type": settings.LD_TAG_MATCH_TYPE,
"tag_fetch_type": settings.LD_TAG_FETCH_TYPE,
}


Expand Down
Loading