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

Add support for telegram chat links opening #397

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

krlade
Copy link
Contributor

@krlade krlade commented Oct 31, 2022

Add support for telegram links opening, fixes #253
For now it only works when the app is already opened, but this might be changed once #4 is completed.

@krlade
Copy link
Contributor Author

krlade commented Nov 3, 2022

I knew that I'll find an issue if I keep digging
Screenshot
In case post argument is supplied telegrand will just fail to open the chat. I'll make a temporary fix and create a separate issue.

@krlade krlade force-pushed the open-links branch 2 times, most recently from 9cc8459 to c34870f Compare November 3, 2022 18:12
@krlade krlade changed the title Draft: Add support for telegram's links opening Draft: Add support for telegram chat links opening Nov 3, 2022
@krlade krlade marked this pull request as ready for review November 3, 2022 18:13
@krlade krlade changed the title Draft: Add support for telegram chat links opening Add support for telegram chat links opening Nov 3, 2022
src/session_manager.rs Outdated Show resolved Hide resolved
src/session_manager.rs Outdated Show resolved Hide resolved
src/application.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@melix99 melix99 left a comment

Choose a reason for hiding this comment

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

Another thing, I usually prefer to have commits grouped per feature. In your case you divided them for single file, which is not that useful, so please squash them into a single one.

@krlade krlade marked this pull request as ready for review March 9, 2023 22:30
@krlade
Copy link
Contributor Author

krlade commented Mar 9, 2023

It's ready for the review. I rewrite my code using official API this time 😁

@melix99 melix99 added this to the Telegrand 0.1 milestone Mar 9, 2023
pub(crate) fn handle_telegram_link(&self, link: String) {
let client_id = self.active_logged_in_client_id();
if let Some(client_id) = client_id {
spawn(clone!(@weak self as obj => async move {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having all these spawn calls, I think it's better to have this whole function (and open_chat_by_username) async and then having the callers of this function to spawn a future to execute it.

if let Ok(link_type) = result {
use enums::InternalLinkType::*;
// TODO: Support other link types
#[allow(clippy::single_match)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to do that, just follow what clippy says. I know that we'll need a match arm in the future, but I prefer not ignore clippy :)

@krlade krlade marked this pull request as draft March 21, 2023 20:59
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.

Support Telegram chat links opening
2 participants