feat: forceSticker viewtype #4814

Septias · 2023-10-10T12:08:12Z

This PR adds a new Viewtype ForceSticker to be able to send any image as a sticker.
It also adds a new parameter to the json-rpc' function send_sticker so that deltachat-desktop can force send images as stickers.

The ForceSticker Viewtype is only used on the sending side and is received as Sticker type on the receiving end.
Maybe we should turn the ForceSticker type into a normal Sticker after recode_to_image_size call to eliminate some redundant (type == Viewtype::Sticker || type == Viewtype::ForceSticker) but I'm not too opinionated on that.

fixes #4739

Septias · 2023-10-10T12:09:26Z

deltachat-jsonrpc/src/api/mod.rs

@@ -1750,10 +1750,15 @@ impl CommandApi {
        account_id: u32,
        chat_id: u32,
        sticker_path: String,
+        force: bool,


This could also be a new function to not break desktop, but I think this adjustment is fine @Simon-Laux

yeah as long as it is mentioned as breaking api change in the changelog

Another option is to always force viewtype in JSON-RPC as we do not need heuristics on desktop currently.

Septias · 2023-10-10T12:48:24Z

Maybe it is actually better to use a specific param keyword for this because then we don't have this problem that Sticker and ForceSticker is logically the same thing but has to be written out everywhere in code.

Simon-Laux · 2023-10-10T13:11:42Z

I guess this is the wrong way to go about it, we should move the transparency detection to the ui and let the UI choose the message type. Like UI should figure out if it is meant as image or as sticker, otherwise we get a mess in the core code and do complicated workarounds like this pr.
If unsure if it makes sense to look for transparent pixels in ui code then let core expose a context less function that takes an image and checks for transparent pixels in the cffi interface.

CC @link2xt @r10s

Septias · 2023-10-10T13:14:32Z

@Simon-Laux Thats what I also thought, but I think the idea behind #4619 was to fix this problem for both IOS and Android.

r10s · 2023-10-10T14:00:14Z

I think the idea behind #4619 was to fix this problem for both IOS and Android.

yip, moving the "smart guessing logic" to the different platforms would have been more work, more code and maybe also more dependencies. it is better done in core where we can also tweak decisions more easily. this also seems performance-wise better as we're recoding things anyways and already have access to the pixel data.

#4619 still improves things hugely on android/ios for keyboards, share-to etc. and it was clear from the beginning on that there is some collateral damage of some stickers becoming images

The ForceSticker Viewtype is only used on the sending side and is received as Sticker type on the receiving end.

+1

Maybe we should turn the ForceSticker type into a normal Sticker after recode_to_image_size call to eliminate some redundant (type == Viewtype::Sticker || type == Viewtype::ForceSticker) but I'm not too opinionated on that.

yip, i would do that as well.

ForceSticker should never be sent out nor be added to the database or persisted otherwise.

ForceSticker should be just a flag that skips some logic. that way, we can always change this part in the future - or maybe even now: a dc_msg_force_viewtype() seems more straight forward, is also closer to what is used in the jsonrpc and may become handy also to skip other smart logics (if needed)

Septias · 2023-10-10T14:07:37Z

so this dc_msg_force_viewtype() should set a Param Param containing the forced type? So that it is accessible everywhere in code where a msg is handled? @r10s

r10s · 2023-10-10T14:43:51Z

so this dc_msg_force_viewtype() should set a Param Param containing the forced type? So that it is accessible everywhere in code where a msg is handled?

my idea is that dc_msg_force_viewtype() has no parameters (beside msg object) and only sets a boolean that is checked then where the conversion sticker->image takes places.

sth. as (cffi)

/** 
 * Sets a flag that avoids changes of the viewtype set by dc_msg_set_viewtype().
 * Otherwise, the viewtype may be changed from STICKER to IMAGE based on file content.
 * Call this function only if you are 100% sure about the type to be sent out.
 */
void dc_msg_force_viewtype(dc_msg_t*);

i think, there is no need to use Param for that, as Param are also persisted and this seems not to be needed. a normal, memory-only flag in Message should do.

that is pretty close to how jsonrpc already works in this pr, the additional viewtype is also hidden there.

Simon-Laux · 2023-10-10T15:52:56Z

yip, moving the "smart guessing logic" to the different platforms would have been more work, more code and maybe also more dependencies. it is better done in core where we can also tweak decisions more easily. this also seems performance-wise better as we're recoding things anyways and already have access to the pixel data.

Doing this opaquely in core makes things harder to understand, if you want to use core, what speaks against exposing a core method to do the transparency check?

If passing the image to core is the problem, then we could make it a method that works on draft messages.

We could then expose an additional call to change the view type of a draft message, that I want to have anyway for the desktop composer update. (or is changing the viewtype possible meanwhile?)

Also in the ui you might have more information, like where and how was it pasted and stuff like that. So my proposal here that detection magic can still happen in core, but the ui will call it and have the last say.

r10s · 2023-10-10T16:20:15Z

what speaks against exposing a core method to do the transparency check?

my suggestions were meant mainly about not persisting another viewtype and about improving the current pr that picks up the suggestions at #4739. however:

surely there are many other ways to do things. the current pr adds mainly a simple if () at the correct position and moves a flag around. this seem to be a pretty easy way forward to fix #4739 without larger changes. a pragmatic approach.

adding sth. as dc_guess_viewtype_from_file() and/or dc_guess_viewtype_from_blob() would probably be more work - and may result in worse performance as the file needs to be decompressed twice - this is maybe often a minor thing, but still, PNG can be quite big, devices can be slow. also, this would be more work on android/ios than just adding a flag or a different viewtype - files/blobs as parameters are usually a little more complicated, esp. as we do not have a really great way to handle them currently (wrt drafts: iirc drafts are not used for keyboard-stickers, so that would also require more changes, and does not look simpler than this pr overall; and the double-decompression would probably still be needed)

just my 2ct, i think, the final decision is up to @link2xt

iequidoo · 2023-10-11T03:41:33Z

I think the PR makes sense, the only thing -- i'd suggest to rename Viewtype::Sticker to MaybeSticker and ForceSticker to just Sticker (for me it is more consistent naming). But to be honest, i don't like much what was done in #4619, actually it's a crutch. If we can't detect in UI if a file is an image or a sticker using its path, i'd prefer to use Viewtype::Image w/o any heuristics.

iequidoo

Maybe better to introduce smth like force_viewtype: bool in Message? Then if it's false, Message::viewtype is just a hint

iequidoo · 2023-10-11T03:57:09Z

src/chat.rs

@@ -2207,11 +2207,12 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> {
            .await?
            .with_context(|| format!("attachment missing for message of type #{}", msg.viewtype))?;

-        let mut maybe_sticker = msg.viewtype == Viewtype::Sticker;
+        let mut maybe_sticker =
+            msg.viewtype == Viewtype::Sticker || msg.viewtype == Viewtype::ForceSticker;
        if msg.viewtype == Viewtype::Image || maybe_sticker {
            blob.recode_to_image_size(context, &mut maybe_sticker)


I'd better pass viewtype here then. Otherwise it can be processed/recoded as an image while being a sticker

Should this function be called at all for stickers that cannot be images?

I think yes, it removes Exif from all files also

iequidoo · 2023-10-11T03:58:41Z

src/chat.rs

@@ -3573,7 +3574,7 @@ pub async fn forward_msgs(context: &Context, msg_ids: &[MsgId], chat_id: ChatId)
        // by not marking own forwarded messages as such,
        // however, this turned out to be to confusing and unclear.

-        if msg.get_viewtype() != Viewtype::Sticker {


Btw, why do we have this logic for stickers?

iequidoo · 2023-10-11T04:01:09Z

src/message.rs

@@ -1858,6 +1858,12 @@ pub enum Viewtype {
    /// A click on a sticker will offer to install the sticker set in some future.


I'd rather add a comment here that it can be converted to Image using some heuristics

Septias · 2023-10-11T07:17:58Z

Maybe better to introduce smth like force_viewtype: bool in Message? Then if it's false, Message::viewtype is just a hint

yeah, that was also @r10s suggestion, will make a new pr later

link2xt

A parameter like "force viewtype" may indeed be better code-wise. Ideally should be in-memory rather than a Param.

It's important to make sure this works with drafts properly, so the flag is not lost when saving a draft with a sticker without transparent pixels and sending it later.

link2xt · 2023-10-11T07:08:40Z

deltachat-jsonrpc/src/api/mod.rs

@@ -1750,10 +1750,15 @@ impl CommandApi {
        account_id: u32,
        chat_id: u32,
        sticker_path: String,
+        force: bool,


Another option is to always force viewtype in JSON-RPC as we do not need heuristics on desktop currently.

link2xt · 2023-10-11T07:11:48Z

src/chat.rs

@@ -2207,11 +2207,12 @@ async fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<()> {
            .await?
            .with_context(|| format!("attachment missing for message of type #{}", msg.viewtype))?;

-        let mut maybe_sticker = msg.viewtype == Viewtype::Sticker;
+        let mut maybe_sticker =
+            msg.viewtype == Viewtype::Sticker || msg.viewtype == Viewtype::ForceSticker;
        if msg.viewtype == Viewtype::Image || maybe_sticker {
            blob.recode_to_image_size(context, &mut maybe_sticker)


Should this function be called at all for stickers that cannot be images?

Septias · 2023-10-11T08:07:39Z

closed in favor of #4819

This approach uses a param field to enable forcing the sticker `viewtype`. The first commit has the memory-only flag implemented, but this flag is not persistent through the database conversion needed for draft/undraft. That's why `param` has to be used. follow up to #4814 fixes #4739 --------- Co-authored-by: Septias <[email protected]>

Septias added 2 commits October 10, 2023 14:01

feat: forceSticker viewtype

9db6da7

api: Add force parameter to send_sticker jsonrpc

293c5ad

Septias requested review from link2xt, Simon-Laux and iequidoo October 10, 2023 12:08

Septias commented Oct 10, 2023

View reviewed changes

iequidoo reviewed Oct 11, 2023

View reviewed changes

link2xt reviewed Oct 11, 2023

View reviewed changes

Septias mentioned this pull request Oct 11, 2023

api(json-rpc): force stickers to be sent as stickers #4819

Merged

Septias closed this Oct 11, 2023

Septias deleted the sk/force_send_sticker branch December 14, 2023 10:54

Septias restored the sk/force_send_sticker branch December 14, 2023 10:54

Septias deleted the sk/force_send_sticker branch December 14, 2023 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: forceSticker viewtype #4814

feat: forceSticker viewtype #4814

Septias commented Oct 10, 2023 •

edited

Loading

Septias Oct 10, 2023

Simon-Laux Oct 10, 2023

link2xt Oct 11, 2023

Septias commented Oct 10, 2023

Simon-Laux commented Oct 10, 2023 •

edited

Loading

Septias commented Oct 10, 2023

r10s commented Oct 10, 2023 •

edited

Loading

Septias commented Oct 10, 2023

r10s commented Oct 10, 2023 •

edited

Loading

Simon-Laux commented Oct 10, 2023 •

edited

Loading

r10s commented Oct 10, 2023 •

edited

Loading

iequidoo commented Oct 11, 2023

iequidoo left a comment

iequidoo Oct 11, 2023

link2xt Oct 11, 2023

iequidoo Oct 11, 2023

iequidoo Oct 11, 2023

iequidoo Oct 11, 2023

Septias commented Oct 11, 2023

link2xt left a comment

link2xt Oct 11, 2023

link2xt Oct 11, 2023

Septias commented Oct 11, 2023

		@@ -1858,6 +1858,12 @@ pub enum Viewtype {
		/// A click on a sticker will offer to install the sticker set in some future.

feat: forceSticker viewtype #4814

feat: forceSticker viewtype #4814

Conversation

Septias commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Septias commented Oct 10, 2023

Simon-Laux commented Oct 10, 2023 • edited Loading

Septias commented Oct 10, 2023

r10s commented Oct 10, 2023 • edited Loading

Septias commented Oct 10, 2023

r10s commented Oct 10, 2023 • edited Loading

Simon-Laux commented Oct 10, 2023 • edited Loading

r10s commented Oct 10, 2023 • edited Loading

iequidoo commented Oct 11, 2023

iequidoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Septias commented Oct 11, 2023

link2xt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Septias commented Oct 11, 2023

Septias commented Oct 10, 2023 •

edited

Loading

Simon-Laux commented Oct 10, 2023 •

edited

Loading

r10s commented Oct 10, 2023 •

edited

Loading

r10s commented Oct 10, 2023 •

edited

Loading

Simon-Laux commented Oct 10, 2023 •

edited

Loading

r10s commented Oct 10, 2023 •

edited

Loading