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

WIP: Allow to create private transfers #14

Closed

Conversation

ilpianista
Copy link

I don't know if this PR is ready, my goal is to allow to hide transfers from the UI when the client applications sets this new private field to true.

To make things easier, transfers are still kept in the database, but don't appear in the Transfers list. This should be enough from a privacy point of view IMHO. However, we can also think of a way to clear private Transfers from the DB (maybe by adding a new method?).

@pvuorela
Copy link
Contributor

What kind of use case do you have in mind? I'm wondering if the transfer is not wanted to be shown on the UI, why inform the transfer-engine at all?

@ilpianista
Copy link
Author

What kind of use case do you have in mind? I'm wondering if the transfer is not wanted to be shown on the UI, why inform the transfer-engine at all?

Downloads from the browser in private mode. I still want to download it, open the file, etc... but I don't want to see it in the transfer list one week later.

@pvuorela
Copy link
Contributor

Ok, there could be two options:

  • Since the browser is doing the download, it could create a separate progress notification and skip the whole transfer-engine. This would be more private, but has some danger of having inconsistent progress notification style etc.
  • OR: do it like here, marking the transfer as private. But then again, if the transfer is not intended to be shown on transfer lists etc, do we need to add it to the database at all? That's also kind of a privacy violation even if the UI doesn't show the item.

Also there's the 'show transfers' action on finished download. If the download is not shown there, the action should be skipped.

If you intend to do this for the browser, would be nice having a PR for that side too.

@@ -45,6 +45,7 @@
<arg direction="in" type="as" name="callback"/>
<arg direction="in" type="s" name="cancelMethod"/>
<arg direction="in" type="s" name="restartMethod"/>
<arg direction="in" type="b" name="privateTransfer"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit vary of breaking the D-Bus and c++ api here. One alternative could be adding separate methods for the private thing.

Referring the comment I made about whether this should be in the database: if this doesn't end up in the database, it could be maybe called 'transient' instead of 'private. There could be reasons to use it for transfers which are not per se private but would be better left out of transfer log anyway.

@ilpianista ilpianista changed the title Allow to create private transfers WIP: Allow to create private transfers Mar 28, 2024
@ilpianista
Copy link
Author

* OR: do it like here, marking the transfer as private. But then again, if the transfer is not intended to be shown on transfer lists etc, do we need to add it to the database at all? That's also kind of a privacy violation even if the UI doesn't show the item.

I like this option more. Harder maybe, but cleaner in my opinion and could be used by others as well.

From a quick look I thought that the database was also needed to share the transfer details (eg. its status) between notifications, UI etc... If it isn't needed, then I can try to skip it for transient transfers.

Also there's the 'show transfers' action on finished download. If the download is not shown there, the action should be skipped.

Noted. Thanks!

If you intend to do this for the browser, would be nice having a PR for that side too.

Sure thing, that's in my TODO as well.

@pvuorela
Copy link
Contributor

From a quick look I thought that the database was also needed to share the transfer details (eg. its status) between notifications, UI etc... If it isn't needed, then I can try to skip it for transient transfers.

I'll confess that I didn't check the details closely on this, but could assume that's doable if not already.

Transient transfers are still kept in the database, but don't appear in the Transfers list
@ilpianista
Copy link
Author

I've renamed private to transient and I've kept the compatibility with DBus/C++ (even if I'm not 100% happy of this solution because I would have liked to use method overloads, but that's not possible with DBus).

About the DB requirement, I see here that transfers' rowId is used as reference in the whole transfer-engine. Should I go ahead and split this dependency? Maybe that should be a different PR implemented before this one?

<arg direction="in" type="s" name="filePath"/>
<arg direction="in" type="s" name="mimeType"/>
<arg direction="in" type="x" name="expectedFileSize"/>
<arg direction="in" type="b" name="transient"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

If the method is "createTransient..." it should already imply that transient is true. So either the code should be adjusted to call either createDownload or this method, or rename this method not to imply transient.

@pvuorela
Copy link
Contributor

pvuorela commented Apr 8, 2024

About the DB requirement, I see here that transfers' rowId is used as reference in the whole transfer-engine. Should I go ahead and split this dependency? Maybe that should be a different PR implemented before this one?

So if the transient is about notification-only, I wouldn't like to have it adding anything to the database. But without that I'm not sure do we have reasons not to use these sql insert ids. Perhaps that dependency split could be part of this PR.

But now thinking further, would it actually still suffice if the private/transient transfers are done as normal transfers, but the client would at the end just call clearTransfer() to remove the entry?

@ilpianista
Copy link
Author

ilpianista commented Apr 8, 2024

But now thinking further, would it actually still suffice if the private/transient transfers are done as normal transfers, but the client would at the end just call clearTransfer() to remove the entry?

Yeah, this definitely makes sense. I could just implement that part in the browser side then 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants