Skip to content

add a device-chat #794

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

Merged
merged 12 commits into from
Nov 6, 2019
Merged

add a device-chat #794

merged 12 commits into from
Nov 6, 2019

Conversation

r10s
Copy link
Contributor

@r10s r10s commented Nov 3, 2019

we want to have a system-"chat" for various purposes:

  • showing update information there
  • adding hints eg. about how to optimize battery, multi-device hints etc. - things that we find out during runtime
  • messages are generated totally offline, by the app itself, we have to make that clear in a message we add to the channel then.
  • the idea is that users in a chat tool are very aware of chatting - a message in an alert or even a sticky thing is easily clicked away. with a chat, the user can also come back later to an issue.

from the ui point of view, the system-chat is a normal chat, not many special things are needed. this pr currently only prototypes the ffi, however, the implementation of these functions should be pretty simple.
and, nothing prevents us to add a simple image here and there :)

once done, we can have more top level-things on that, eg. a method to add things only once to the channel or a update-info and so on.

wrt naming: any suggestions are welcome - otherwise, i fear we stick to the name "clippy" :)

nb: beside DC_CONTACT_ID_CLIPPY, there is aready another "system" user, DC_CONTACT_ID_DEVICE that is the user-id of the centered messages as "member added", "setup changes" etc. as these messages may also appear inside the clippy-chat, we need an extra contact-id.

@hpk42
Copy link
Contributor

hpk42 commented Nov 4, 2019

I really like "DC_CONTACT_ID_DEVICE" and thus would suggest to talk about "device chat" -- it's much better than clippy which invokes an annoying remote data-tracking AI-agent ... or conversely the "cargo clippy" thingie. This is user-visible how we talk about it in FAQs etc. Device chat emphasizes that it's just your local system informing you of something (also tips like "turn on background service" etc.) or suggesting to do a backup after you have 100 chat messages or 10 chats or so. The AI part can then come later but would run fully local at least ;)

Copy link
Contributor

@hpk42 hpk42 left a comment

Choose a reason for hiding this comment

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

this looks fine though i hope you do a global replace clippy->device ;)

@r10s
Copy link
Contributor Author

r10s commented Nov 4, 2019

the problem is that DC_CONTACT_ID_DEVICE is already in use for the "member added" etc. but it seems no to be used by the ui (they use dc_msg_is_info()), we can maybe rename it to DC_CONTACT_ID_INFO or so.

@r10s r10s changed the title prototype a system-chat [wip] prototype a system-chat Nov 4, 2019
@r10s
Copy link
Contributor Author

r10s commented Nov 4, 2019

okay, the things are named DC_CONTACT_ID_DEVICE (5) and dc_chat_is_device_talk() now.

the old DC_CONTACT_ID_DEVICE (2) is now DC_CONTACT_ID_INFO (2) - this is only an internal rename, at least our uis use dc_msg_is_info() and also DC_CONTACT_ID_DEVICE was never documented somewhere before.

@r10s r10s force-pushed the clippy2 branch 7 times, most recently from 0d791d0 to 4f18be9 Compare November 4, 2019 19:36
@r10s r10s changed the title [wip] prototype a system-chat add a device-chat Nov 4, 2019
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

I am surprised the device chat is a Param on the Chat rather than a special chat id. Why this way?

@@ -811,6 +811,23 @@ pub unsafe extern "C" fn dc_set_draft(
.unwrap_or(())
}

#[no_mangle]
pub unsafe extern "C" fn dc_add_device_msg(context: *mut dc_context_t, msg: *mut dc_msg_t) -> u32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a libc::?? return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, i did just a copy+paste from dc_chat_can_send() :)

i leave it for now, maybe we can go trough the ffi and streamline this at some point for all affected functions.

Copy link
Collaborator

@link2xt link2xt Nov 5, 2019

Choose a reason for hiding this comment

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

libc::uint32_t is deprecated in favor of u32 in Rust. And using u32 is better than libc::c_uint because JNI and NAPI don't work these C types anyway.

Keep it returning u32 and specify it as uint32_t in C API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to streamline it, I would rather change all ints to int32_t, because it is equal to int32_t on all existing architectures anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for explaining, @link2xt

* @param chat The chat object.
* @return 1=chat is writable, 0=chat is not writable
*/
int dc_chat_is_writable (const dc_chat_t* chat);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is a weird name, to me a non-writable chat would be one which can not have any messages in it ever.
maybe dc_chat_can_send or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yip, this sounds better. i will change it.

@@ -677,8 +691,7 @@ pub fn msgtype_has_file(msgtype: Viewtype) -> bool {
}
}

fn prepare_msg_common(context: &Context, chat_id: u32, msg: &mut Message) -> Result<MsgId, Error> {
msg.id = MsgId::new_unset();
fn prepare_msg_blob(context: &Context, msg: &mut Message) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nice refactor

prepare_msg_blob(context, msg)?;
unarchive(context, chat_id)?;

context.sql.execute(
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this a custom insert? I appreciate you don't want to call send_msg() but i'm surprised to see a whole new sql query. I probably missed something though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is just no other INSERT INTO msgs that would only do an insert. the ones in receive_imf(), set_draft(), set_info_msg() do other, unwanted stuff and/or have a customized insert on their own.

but i think this is no issue. these sqls are easy enough and so it is pretty clear what happens.

@r10s
Copy link
Contributor Author

r10s commented Nov 4, 2019

I am surprised the device chat is a Param on the Chat rather than a special chat id. Why this way?

i was also thinking about using a DC_CHAT_ID_DEVICE ...

  • i think it is easier this way as there are many cases checking for DC_CHAT_ID_LAST_SPECIAL - so many things would not work with a DC_CHAT_ID_DEVICE out of the box and need adaptions.
  • this would also affect many sql-statements
  • the device-chat is a pretty normal chat from the view of the ui
  • if think the device-chat is similar to the me-chat (aka saved-messages) - so it is nice to have a similar behavior here
  • we save one special-chat-id :)

so, these special-ids are more for chats that should not appear in mosts list and are no real chats in reality - eg. the virtual deaddrop, the archive-link etc.

all in all, it's easier for the device-chat to be a "normal" chat.
(in contrast to the device-contact: here we use DC_CONTACT_ID_DEVICE as it shall not appear as a "normal" contact in lists, shall not be blockable or deletable etc.)

@link2xt
Copy link
Collaborator

link2xt commented Nov 5, 2019

Is there any place to read initial discussion on this feature? Was it on IRC? Completely missed that.

@r10s
Copy link
Contributor Author

r10s commented Nov 5, 2019

Is there any place to read initial discussion on this feature? Was it on IRC? Completely missed that.

it was mostly offline and on irc iirc :)

@r10s
Copy link
Contributor Author

r10s commented Nov 5, 2019

nb: the current device-chat-logo is ugly, i have an idea and will replace it. @hpk42 so maybe wait with merging.

@r10s r10s force-pushed the clippy2 branch 2 times, most recently from 59c06ed to e654874 Compare November 5, 2019 22:28
@r10s r10s changed the title add a device-chat [wip] add a device-chat Nov 5, 2019
@r10s r10s changed the title [wip] add a device-chat add a device-chat Nov 6, 2019
@r10s
Copy link
Contributor Author

r10s commented Nov 6, 2019

this is what it looks like now:

Screen Shot 2019-11-06 at 02 16 47

@hpk42 hpk42 merged commit 515f0c5 into master Nov 6, 2019
@hpk42
Copy link
Contributor

hpk42 commented Nov 6, 2019

great work, looking forward to seeing it in action ;)

@r10s r10s deleted the clippy2 branch November 6, 2019 13:47
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.

4 participants