Skip to content

globally search for media#3528

Merged
r10s merged 8 commits into
masterfrom
get-all-media
Aug 18, 2022
Merged

globally search for media#3528
r10s merged 8 commits into
masterfrom
get-all-media

Conversation

@r10s

@r10s r10s commented Jul 28, 2022

Copy link
Copy Markdown
Contributor

this pr allows passing 0 to dc_get_chat_media(), the function returns media from any chat then - similar to dc_search_msgs().

moreover, this pr fixes a minor issue to not return hidden media - and a test (dc_get_chat_media() was completely untested)

closes #3527

@r10s r10s requested review from Hocuri, cyBerta and link2xt July 28, 2022 22:59
Comment thread deltachat-ffi/deltachat.h Outdated
@r10s

r10s commented Jul 29, 2022

Copy link
Copy Markdown
Contributor Author

one thing @adbenitez pointed out in dev chat, is that this approach does not care for duplicates - in a selector you maybe do not want to have them. however, unless there is a dead-simple approach, to allow UI to move forward, it probably makes sense to care for that at some later point in a subsequent pr. also, there is the rough idea of files deduplication, this may help somehow as well ;)

Comment thread deltachat-jsonrpc/src/api/mod.rs Outdated
Comment on lines +590 to +596
let chat_id = if chat_id == 0 {
None
} else {
Some(ChatId::new(chat_id))
};

@Simon-Laux Simon-Laux Aug 15, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

api wise I would prefer changing line 583 to chat_id: Option<u32>
and then using map to convert it:

Suggested change
let chat_id = if chat_id == 0 {
None
} else {
Some(ChatId::new(chat_id))
};
let chat_id = chat_id.map(|id| ChatId::new(id));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that way I can pass null in the js api instead of the number 0 which is a bit more clear IMO

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but not too important, its already ok

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

api wise I would prefer changing line 583 to chat_id: Option
and then using map to convert it:

feel free to push to this pr or do a subsequent one - i was changing up to the existing api only as i do not want to touch the desktop code at all and keep this pr scoped to the rust core part.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mentioned it mostly that you know that its an option too, Option<type> gets translated basically to type | null

@r10s

r10s commented Aug 17, 2022

Copy link
Copy Markdown
Contributor Author

rust fmt is unhappy now :)

EDIT: pushed a fix.

@r10s

r10s commented Aug 18, 2022

Copy link
Copy Markdown
Contributor Author

k, then let's merge that in to go forward with deltachat/deltachat-ios#1668 :)

@r10s r10s merged commit fdf91b7 into master Aug 18, 2022
@r10s r10s deleted the get-all-media branch August 18, 2022 08:27
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.

global search for media

3 participants