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
Feature/custom emoji and tagline views #4580
base: main
Are you sure you want to change the base?
Feature/custom emoji and tagline views #4580
Conversation
We can publish this in 0.19.x, and then remove the |
I've added the breaking change label, so we'll wait till after this upcoming release to merge this. Its unavoidably a breaking change tho, because taglines and emojis are going to come back with separate endpoints. @Nutomic I'd rather just push this to 0.20 |
#[cfg_attr(feature = "full", ts(export))] | ||
/// Fetches a list of registration applications. | ||
pub struct ListCustomEmojis { | ||
pub page: Option<i64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makotech222 look into your emoji library, because these may need a search term, optional category, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a category search parameter. This column is also already indexed
Confirming: This means we'll remove the |
yes. IMO a single random single tagline can could back with getsiteresponse also, so that front ends can use a random tagline. But proper editing of taglines would be the new endpoints. |
Understood. I'll also need to add the create, update and delete endpoints for taglines then indeed. |
I've added new endpoints to edit taglines, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
Also can you mark this as a draft PR? I'm mainly afraid of one of us prematurely merging it, since it has some breaking changes. After this upcoming release, we'll be doing breaking changes again, and one of us can mark it as ready for review.
Remove superfluous properties
2d7272e
to
6ffcc1f
Compare
@@ -22,6 +22,8 @@ const BIO_MAX_LENGTH: usize = 300; | |||
const ALT_TEXT_MAX_LENGTH: usize = 300; | |||
const SITE_NAME_MAX_LENGTH: usize = 20; | |||
const SITE_NAME_MIN_LENGTH: usize = 1; | |||
const TAGLINE_CONTENT_MIN_LENGTH: usize = 1; | |||
const TAGLINE_CONTENT_MAX_LENGTH: usize = 50000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems extremely long, same as max post body. I think 200 or so would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw only is_valid_post_title()
calls trim()
before length check. Would probably make sense to trim
inside of min_length_check
and min_length_check
so that it applies to all strings.
cc @dessalines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hexbear uses taglines far longer than 200 characters. Lemmygrad as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took 50000 which is the default max body length. Seems like too much indeed. Maybe 500 chars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked hexbear and lemmygrad taglines manually, longest one I noticed is 1170 chars. That seems extremely long but if we want to support it, we can set a limit of 1500 or 2000 chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we shouldn't be too opinionated in the back-end about the limits. If sites want to take up an entire screen with a tagline, that's up to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed @dessalines. Shall we keep it as is, or remove the max limit completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is seems fine to me. You've externalized it so we can always tweak it later easily.
) | ||
.into_boxed(); | ||
|
||
query = query.filter(custom_emoji::local_site_id.eq(for_local_site_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary because all emojis have the same local site id. Honestly you can remove the local_site_id
column, it doesnt seem to have any use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a similar point regarding taglines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the local_site_id
from both custom_emoji
and tagline
. It's all done in a single commit, so it should be easy to revert if it turns out to be needed some reason.
) | ||
.into_boxed(); | ||
|
||
query = query.filter(custom_emoji::local_site_id.eq(for_local_site_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a similar point regarding taglines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need feedback from @makotech222 , because we need to tie in how async loading of emojis, and searching, will work in the front ends.
Like i said, I don't think separating out the emoji list into pages is a worthwhile endeavor. Taglines is fine, because we technically only need a random one on page load and then you can paginate for admin page. Its up to you guys how you want to do the frontend side if you paginate emojis. You'd have to make sure Tribute and EmojiMart can both support it. Just remember its going to require a network call + a db call to handle each and every search anytime emojis are used. |
I only gave the Tribute and EmojiMart code a quick look. It does seem that for EmojiMart all emojis needs to be loaded when constructing the I think having the paged API for custom emojis is fine to have for the admin functionality. |
Dang. Well let's allow an override on the paging limits on the new In the future tho we should look to either add async to emojimart, or find a new library, because fetching thousands of emojis all at once isn't going to scale up well. |
Just want to note, hexbear probably has the most expansive emoji and tagline list in lemmyverse. Our getsiteresponse in its entirety is about 1.7mb. Not that bad |
EmojiPicker needs to be able to retrieve all emojis in 1 call
The only thing I could find was on the |
I noticed that on the |
Yes it would be much simpler with caching. Caching different pages would be possible with cache key |
Its not just about the fetch size, its also about limiting the amount of info a front end page has to render and display. Let's say you're editing emojis and taglines. You don't want to have to render, load images for, and scroll through 800 emojis all at once. As for caching, I think that's on us to add it if we think its necessary, not you. |
@@ -35,16 +35,49 @@ impl CustomEmojiView { | |||
} | |||
} | |||
|
|||
pub async fn get_all( | |||
pub async fn get_all(pool: &mut DbPool<'_>) -> Result<Vec<Self>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably get rid of this one now. Or leave it as a helper function, but have it call `self.list(.... true)
query = query.filter(custom_emoji::category.eq(category)) | ||
} | ||
|
||
let emojis = query | ||
.order(custom_emoji::category) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The category ordering should probably only happen if they supplied a category, just to be faster. So you could add .order_by(category)
into the if let Some(category)
above
then add query = query.then_order_by(id)
below that block.
Keeping pagination is also fine. In that case it makes no sense to do caching in Rust, better leave it to nginx. That is already handled by cache-control headers which Lemmy sends for unauthenticated users. |
Only keep get_all als helper function calling list with paging ignored Only order on category when filtering on category
Sounds good to me.
I'll keep the the paging as it is then |
Add paged endpoints for
custom_emoji
andtagline
, as per #4577Does not remove the resources from
GetSiteResponse
.@dessalines:
GetSiteResponse
? Cancustom_emojis
andtaglines
only be removed onapi/v4
?site
update endpoint. Do you want complete separation of this resource, like withcustom_emoji
?