KC3Kai / kancolle-replay

Sortie Replay and Simulation by @fourinone41
MIT License
51 stars 33 forks source link

Shared link generation #80

Closed X-20A closed 2 weeks ago

X-20A commented 2 weeks ago

I often see comments stating that others cannot reproduce the computation results (often because only screenshots of the results are shared). To address this issue, having a feature similar to ACSim's [Share] > [Create share link URL] would be beneficial, in my opinion.

I have written a rough description of the expected process flow.Gist

In short, I think LZMA compression and then base64 encoding will result in a string of a size that will ride on the URL. There are several ways to generate shortened URLs, but Firebase Dynamic Links seems to be deprecated.

X-20A commented 2 weeks ago

P.S. How about adding a screenshot feature? sample キャプチャ_2024_07_09_22_27_00_1 screenshot (8)

fourinone41 commented 2 weeks ago

The current way to share is to use .kcsim backup files, but I guess that may be inconvenient for sharing on Twitter? Is that the use case?

big.kcsim I tested with a purposely big sim setup and reached 7532 url length. It seems tinyurl api still accepts it, but I wonder where you got 4096 limit? 57-7.kcsim I also averaged 4000~5000 on medium/realistically sized setups, not really close to ~2600 that you're getting?

X-20A commented 2 weeks ago

The current way to share is to use .kcsim backup files, but I guess that may be inconvenient for sharing on Twitter? Is that the use case?

Yes, Twitter does not support uploading of .kcsim files, so sharing of simulations is done exclusively by screenshots as far as I know.

I wonder where you got 4096 limit?

Here it is. In Safari, it seems that if the url exceeds 4096 characters, the url cannot be copied, but if I read carefully, we can get it in js, and the url in key: backup is not likely to be copied directly, so it may not have been a problem.

X-20A commented 2 weeks ago

https://kc3kai.github.io/kancolle-replay/?share=... I guess I can make the url even more compact by redirecting here as

fourinone41 commented 2 weeks ago

I see, so it's Safari's limit, but not Chrome/Firefox's. Sim already uses # fragment for replay import and direct sim import, both of which already may exceed 4096, so since this is the same browser issue, I think it's OK for now.

Do you have JP term for "Create Shared Link (TinyURL)"?

X-20A commented 2 weeks ago

"Create Shared Link (TinyURL)": "共有URLを生成(TinyURL)", but the user can use like "https://kc3kai.github.io/kancolle-replay/simulator.html/?share=xk2qjfz", so I don't think it is necessary to specify tinyurl.

fourinone41 commented 2 weeks ago

Actually I plan to output the TinyURL link directly, better not to hide it, in my opinion. For example: click button -> copies https://tinyurl.com/29pdrhkl to clipboard -> post that link directly

Also will need change log, probably something like Create Shared Link (TinyURL), screenshot button for Statistics

X-20A commented 2 weeks ago

better not to hide it

Why is that? It is common to use a shortened URL parameter for another URL, and if the domain is tinyurl, it will be impossible to determine what site's url is from the url. example: キャプチャ_2024_07_10_19_33_01_255

changelog: "共有リンク(TinyURL)生成機能と演算結果のスクリーンショットボタンの実装"

fourinone41 commented 2 weeks ago

Well no matter what, you are opening an arbitrary tinyurl link in the end (location.href =), so it's better to present it to people as is, so they know it's an arbitrary link and may make their own judgement on whether or not to trust that link, scope it out, etc. Otherwise you can trick people into opening any malicious tinyurl link by disguising it as a simulator.html link. Aircalc can do this because they have server-side component, they can just store your data without external dependencies, and can't be abused.

X-20A commented 2 weeks ago

Sure, I understand. It's indeed a tricky situation. Adding a "+" at the end like this "location.href = 'https://tinyurl.com/2xk2qjfz+'" during redirection seems to ensure passing through a confirmation page. What do you think?

X-20A commented 2 weeks ago

It seems to be safe in that it must go through githubpages, right?

fourinone41 commented 2 weeks ago

That does make it safer I think, I'll add it to the output. But still not really a fan of using simulator url, as long as this feature isn't natively supported. I think it won't be too confusing anyway, since you'll usually have context when sharing it? I don't know, maybe I'll change my mind later, but for now I'll stay with outputting tinyurl directly.

fourinone41 commented 2 weeks ago

One more thing: message needs to be adjusted to include all import types now, not just replay Warning: Import will overwrite existing data. 警告: リプレイからのデータのインポートにより既存のデータが上書きされます。

X-20A commented 2 weeks ago

警告: データのインポートにより既存のデータが上書きされます。