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

Adjust field in LinkCapabilities struct #311

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Jul 22, 2024

This fixes linking.

@gferon gferon merged commit 1b59154 into main Jul 22, 2024
19 checks passed
@gferon gferon deleted the gabriel/fix-capabilities-linking branch July 22, 2024 12:55
// https://github.com/signalapp/Signal-Desktop/blob/1e57db6aa4786dcddc944349e4894333ac2ffc9e/ts/textsecure/WebAPI.ts#L1287
impl Default for LinkCapabilities {
fn default() -> Self {
Self { delete_sync: true }
Copy link
Member

Choose a reason for hiding this comment

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

Uhh, do we actually implement that capability? 🫣

Maybe it was worth setting it to false? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's hardcoded as true in Signal-Desktop so... 🤷🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Yeh okay, probably since SD implements that capability. If we don't make any change, nor we understand what the capability is about, then we might be signing up for a crash-and-burn down the road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair, let me try if it works with false 😄

Copy link
Member

Choose a reason for hiding this comment

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

Well, if we can just find what this means, and document it, that'd also be fine I suppose :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel the default() is pointless here, we could just document the only place it's used in. And we need to know what the flag means before we set it either way.

Copy link
Collaborator Author

@gferon gferon Jul 23, 2024

Choose a reason for hiding this comment

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

I guess it's a matter of taste, I'm not attached to any way of doing it 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, if we can just find what this means, and document it, that'd also be fine I suppose :)

Can confirm that you cannot link when setting the flag to false.

@direc85
Copy link
Contributor

direc85 commented Jul 23, 2024

Just noticed this was merged already. Anyway...

I found out what Signal Android does:

ThreadDelete(
  threadRecipientId = threadRecipient.id.toLong(),
  isFullDelete = isFullDelete,
  messages = messages.map {
    AddressableMessage(
      sentTimestamp = it.dateSent,
      authorRecipientId = it.fromRecipient.id.toLong()
    )
  },
  nonExpiringMessages = nonExpiringMessages.map {
    AddressableMessage(
      sentTimestamp = it.dateSent,
      authorRecipientId = it.fromRecipient.id.toLong()
    )
  }
)

Whisperfish at least already supports individual message deletes, but this feature is about syncing thread deletes i.e. deleting full conversations across devices. That can't be done in libsignal IIUC and so has to be a flag provided by the client and not hardcoded in libsignal-service.

Please change the default to false and provide the API to set the flag. Thanks.

@gferon
Copy link
Collaborator Author

gferon commented Jul 23, 2024

Just noticed this was merged already. Anyway...

I found out what Signal Android does:

ThreadDelete(
  threadRecipientId = threadRecipient.id.toLong(),
  isFullDelete = isFullDelete,
  messages = messages.map {
    AddressableMessage(
      sentTimestamp = it.dateSent,
      authorRecipientId = it.fromRecipient.id.toLong()
    )
  },
  nonExpiringMessages = nonExpiringMessages.map {
    AddressableMessage(
      sentTimestamp = it.dateSent,
      authorRecipientId = it.fromRecipient.id.toLong()
    )
  }
)

Whisperfish at least already supports individual message deletes, but this feature is about syncing thread deletes i.e. deleting full conversations across devices. That can't be done in libsignal IIUC and so has to be a flag provided by the client and not hardcoded in libsignal-service.

Please change the default to false and provide the API to set the flag. Thanks.

false is not an option, since you can't link without it: the support is now mandatory. I find thus the impl Default important.

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.

3 participants