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

gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes #2771, ref #3868) #7984

Merged
merged 51 commits into from
Nov 7, 2022

Conversation

tomasz1986
Copy link
Contributor

@tomasz1986 tomasz1986 commented Oct 1, 2021

Add buttons to allow for simpler sharing device IDs with others. The
first one copies the ID to clipboard (trying to use three different
methods, depending on the browser). The second one triggers a mailto
link with prefilled subject and body. The third one triggers an sms link
with prefilled body. The short description of Syncthing included in the
official website https://syncthing.net.

Issue #3868 is referred here, because the copy to clipboard button
offers an alternative method for IE11 users to actually be able to copy
device IDs without having to select it manually (which doesn't work).

Signed-off-by: Tomasz Wilczyński twilczynski@naver.com

Screenshots

image

image

image

image

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Oct 1, 2021

This is a draft, because I still need to figure out some CSS regarding small resolutions. Right now the buttons don't wrap and overflow the screen on mobile. I'm going to add screenshots and other details once I've fixed this problem.

As far as the email text goes, I've got some inspiration from the linked issue, and I've also checked what Dropbox says in their invitation mails (which are actually quite short, probably assuming that people already know what the software is about). If you've got any ideas how the text could be improved, please let me know in advance 🙂. I've deliberately decided not to include any direct links straight to the Docs and such, because the recipient may be using the Android app or another wrapper that has its own UI, so I've decided to link to the official site only (which has all the required information anyway).

Also, one more thing, is it actually possible to translate text inserted via mailto? It's all written in one line, and also has special formatting of spaces and new lines, so I've got no idea whether this can even be processed by Transifex at all. If not, is there any other way to insert this text, so that it could still be translated?

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Some GUI styling issues can probably be avoided entirely. If you describe what different behavior you'd like to achieve with the CSS, maybe I can help find a solution.

gui/default/index.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/editDeviceModalView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/editDeviceModalView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/idqrModalView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/idqrModalView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/idqrModalView.html Outdated Show resolved Hide resolved
@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Oct 6, 2021

Some GUI styling issues can probably be avoided entirely. If you describe what different behavior you'd like to achieve with the CSS, maybe I can help find a solution.

This is still a work in progress more or less. I'll try to fix the current issues first, and the come back with what's left and ask for more comments and input 🙂.

I'd also welcome very much all opinions on the e-mail text itself. For now, I'm definitely going to remove the The Syncthing Authors signature present there, simply because these e-mails won't be sent by Syncthing, but rather from the user's e-mail address.

@acolomb
Copy link
Member

acolomb commented Oct 6, 2021

The email text sounds fine to me, except I don't know if "invited to Syncthing" conveys enough context. Syncthing is not a place, and not the device is inviting, but the user sending the mail. Suggestion:

You are being invited to synchronize with '<device-name>' using Syncthing!

Note the quotes around the device name. Could also use "collaborate" or "connect" instead of "synchronize".

@st-review
Copy link

🤖 beep boop

I'm going to close this pull request as it has been idle for more than 90 days.

This is not a rejection, merely a removal from the list of active pull requests that is periodically reviewed by humans. The pull request can be reopened when there is new activity, and merged once any remaining issues are resolved.

@st-review st-review closed this Jan 5, 2022
@calmh calmh reopened this Jan 5, 2022
@tomasz1986 tomasz1986 changed the title gui: Add copy to clipboard and send email buttons to device IDs (fixes #2771, fixes #3868) gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes #2771, ref #3868) Feb 9, 2022
Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Thought I'd have a look at the current draft status again. Overall it looks nice, but I spotted some possible improvements. One bigger request, please make this applicable to the local device as well, e.g. in the "Show QR" modal dialog.

gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/device/editDeviceModalView.html Outdated Show resolved Hide resolved
script/translate.go Outdated Show resolved Hide resolved
@tomasz1986 tomasz1986 force-pushed the tomasz86-gui-shareid branch 2 times, most recently from e08b77b to aac7547 Compare April 19, 2022 12:48
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/device/editDeviceModalView.html Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
script/translate.go Outdated Show resolved Hide resolved
@acolomb
Copy link
Member

acolomb commented Apr 19, 2022

I couldn't resist taking a look at the new wip state ;-) Hope you can make use of at least the coding style comments.

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Apr 20, 2022

I couldn't resist taking a look at the new wip state ;-) Hope you can make use of at least the coding style comments.

Thank you very much for all the comments, although you will probably waste some time this way, as I myself am still trying out different approaches 😉.

Edit: Just in case you decide to test the current state, please check only the QR modal, as the buttons in the other one are broken at the moment.

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Some more coding style comments and I think you have a rather technical perspective on the explanation texts. Thus I tried to give some suggestions for more user-friendly and less technical strings.

I really like the new preview dialog overall, explaining what will (hopefully) happen. It's one more click to achieve the end result, but guiding the user through the process might be worth it. Just forget that the reason for it was "I can't detect whether it works", allowing shorter and less confusing instructions.

gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/core/syncthingController.js Outdated Show resolved Hide resolved
gui/default/syncthing/device/shareDeviceDialogView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/shareDeviceDialogView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/shareDeviceDialogView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/shareDeviceDialogView.html Outdated Show resolved Hide resolved
gui/default/syncthing/device/shareDeviceDialogView.html Outdated Show resolved Hide resolved
@uok
Copy link
Contributor

uok commented Apr 25, 2022

You should also take a look at https://developer.mozilla.org/en-US/docs/Web/API/Navigator/share which enables native sharing, e.g. on Android it opens actions like "share with contact on Signal"

I have added it to my own projects with multiple buttons e.g. "email", "facebook" and "share". I then show/hide whatever buttons are supported/not supported.

if (navigator.share) { 
  // sharing supported - hide all buttons except "share"
}
else {
  // sharing not supported - hide share button, keep fallback buttons
}

It works really well!

@bt90
Copy link
Contributor

bt90 commented Aug 2, 2022

@tomasz1986 are you still working on this? Would love to see this merged.

@tomasz1986
Copy link
Contributor Author

@tomasz1986 are you still working on this? Would love to see this merged.

Yeah, still planning to get back to this one sometime in the near future. Thanks for asking 🙂.

As for the the mobile-specific code suggested above, I'll probably leave it to a separate PR that would possibly follow this one, as the PR is already quite extensive, and I'm also worried that if I start to deep dive into the mobile sharing, I'll never be able to finish it.

…to device IDs (fixes syncthing#2771, ref syncthing#3868)

Add buttons to allow for simpler sharing device IDs with others. The
first one copies the ID to clipboard (trying to use three different
methods, depending on the browser). The second one triggers a mailto
link with prefilled subject and body. The third one triggers an sms link
with prefilled body. The short description of Syncthing included in the
latter part of the body is a direct copy from the description at the
official website https://syncthing.net.

Issue syncthing#3868 is referred here, because the copy to clipboard button
offers an alternative method for IE11 users to actually be able to copy
device IDs without having to select it manually (which doesn't work).

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
@tomasz1986
Copy link
Contributor Author

Yeah, according to the specs, the syntax should just be sms:?body=, but this apparently doesn't work in iOS (at least according to the Stackoverflow discussion). I've now tried to remove ;. Please try testing the current version when you've got some free time.

@xor-gate
Copy link
Member

It seems now the To field is empty (instead of ; and the cursor/selection of the input box is now correct:

PNG image

@xor-gate
Copy link
Member

xor-gate commented Sep 19, 2022

On macOS 12.3.1 it also works fine for SMS/message. Also the email share works as expected. Screenshot below is for the SMS share url:

Screenshot 2022-09-19 at 17 43 39

@tomasz1986
Copy link
Contributor Author

That was quick 🙂. It seems we've got macOS and iOS covered then.

The current iteration also works fine for me here on Samsung Android.

acolomb
acolomb previously approved these changes Sep 19, 2022
Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

Confirmed to work on my Google Pixel (Android 13) as well.

Just out of curiosity, is there similar functionality in recent Windows versions? Can e.g. Microsoft's "Surface" tablets with cellular modem send SMS? (Shouldn't block this PR though when we have the two biggest mobile OSes covered.)

@tomasz1986
Copy link
Contributor Author

tomasz1986 commented Sep 19, 2022

Just out of curiosity, is there similar functionality in recent Windows versions? Can e.g. Microsoft's "Surface" tablets with cellular modem send SMS? (Shouldn't block this PR though when we have the two biggest mobile OSes covered.)

Judging by what people write online, the answer is "yes", at least on some devices, but I haven't used any Windows tablets with a modem built in, so I don't really know any details. In theory, the link should work there too.

@xor-gate
Copy link
Member

xor-gate commented Nov 6, 2022

@tomasz1986 because nobody did merge after approval the feature branch has a merge conflict and is now unable to be merged. Could you sync the changes back so the branch is mergeable?

@tomasz1986
Copy link
Contributor Author

@tomasz1986 because nobody did merge after approval the feature branch has a merge conflict and is now unable to be merged. Could you sync the changes back so the branch is mergeable?

Done! Thanks for giving a hint, as I completely forgot about checking the older PRs for conflicts 😳.

@xor-gate xor-gate self-requested a review November 6, 2022 09:55
@xor-gate xor-gate added this to the v1.22.2 milestone Nov 6, 2022
@xor-gate
Copy link
Member

xor-gate commented Nov 6, 2022

@acolomb i'm not involved in the syncthing daemon development. Who is responsible for merging so it can be shipped with the v1.22.2 release window?

@acolomb
Copy link
Member

acolomb commented Nov 6, 2022

I was hesitant to merge because no other maintainer had commented much. And I don't see myself making last calls on features without an opinion from one of them.

@xor-gate
Copy link
Member

xor-gate commented Nov 6, 2022

I was hesitant to merge because no other maintainer had commented much. And I don't see myself making last calls on features without an opinion from one of them.

Totaly agree with this. But LGTM.

@acolomb
Copy link
Member

acolomb commented Nov 7, 2022

After no objections, let's go ahead and try pouring this over the brave RC users...

@acolomb acolomb merged commit 5e384c9 into syncthing:main Nov 7, 2022
calmh added a commit to imsodin/syncthing that referenced this pull request Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
calmh added a commit to calmh/syncthing that referenced this pull request Nov 22, 2022
* main: (23 commits)
  lib/fs: Optimize WindowsInvalidFilename (syncthing#8687)
  gui, man, authors: Update docs, translations, and contributors
  cmd/syncthing: Use main logger in generate subcommand (fixes syncthing#8682) (syncthing#8685)
  build: Update all dependencies (fixes syncthing#8679) (syncthing#8680)
  gui, man, authors: Update docs, translations, and contributors
  lib/model: Correctly set xattrs on temp files (fixes syncthing#8667) (syncthing#8670)
  gui: Automatically dismiss authentication reminder when in LDAP mode (fixes syncthing#8661) (syncthing#8663)
  lib/model: Correctly handle xattrs on directories (fixes syncthing#8657) (syncthing#8658)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  ...
@tomasz1986 tomasz1986 deleted the tomasz86-gui-shareid branch January 8, 2023 11:45
tomasz1986 added a commit to tomasz1986/syncthing that referenced this pull request Jul 27, 2023
As per Bootstrap recommendation, buttons with tooltips inside button
groups require to have container: 'body' set. This prevents tooltips
from causing the buttons to jump on hover and also allows the tooltips
to be wider instead of wrapping on every space.

Ref: https://getbootstrap.com/docs/3.3/components/#btn-groups

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh pushed a commit that referenced this pull request Jul 27, 2023
As per Bootstrap recommendation, buttons with tooltips inside button
groups require to have container: 'body' set. This prevents tooltips
from causing the buttons to jump on hover and also allows the tooltips
to be wider instead of wrapping on every space.

Ref: https://getbootstrap.com/docs/3.3/components/#btn-groups

Signed-off-by: Tomasz Wilczyński <twilczynski@naver.com>
calmh added a commit to calmh/syncthing that referenced this pull request Jul 30, 2023
* main:
  all: Refactor the protocol/model interface a bit (ref syncthing#8981) (syncthing#9007)
  lib/connections: Fix building with `-tags noquic` (syncthing#9009)
  gui: Fix tooltips on buttons inside button groups (ref syncthing#7984) (syncthing#9008)
calmh added a commit to calmh/syncthing that referenced this pull request Aug 3, 2023
* main:
  build: Increase Go version to 1.20.7
  lib/config: Allow sharing already encrypted folder with untrusted devices (fixes syncthing#8965) (syncthing#9012)
  gui: Use case-insensive and backslash-agnostic versions filter (fixes syncthing#7973) (syncthing#8995)
  gui, man, authors: Update docs, translations, and contributors
  build: Run govulncheck (fixes syncthing#8983)
  build: Run build & tests on main branch nightly
  build: Send test logs to Grafana Loki for statistics
  all: Refactor the protocol/model interface a bit (ref syncthing#8981) (syncthing#9007)
  lib/connections: Fix building with `-tags noquic` (syncthing#9009)
  gui: Fix tooltips on buttons inside button groups (ref syncthing#7984) (syncthing#9008)
  cmd/strelaysrv: Handle accept error with debug set (fixes syncthing#9001) (syncthing#9004)
  lib/api: Fix data race in TestCSRFRequired (syncthing#9006)
  gui: Show full error for failed items (syncthing#9005)
  lib/api: Allow `Bearer` authentication style with API key (syncthing#9002)
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 8, 2023
@syncthing syncthing locked and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants