Integrate AnkiHub Sign-in #3232
Conversation
This is used by the add-on.
rslib/src/ankihub/login.rs
Outdated
|
|
||
| #[derive(Serialize, Deserialize, Debug)] | ||
| pub struct LoginRequest { | ||
| // FIXME: we need to pass either `username` or `email` |
There was a problem hiding this comment.
The add-on uses a regex to determine whether the user entered an email or a username, but maybe it makes more sense to move the logic to the AnkiHub backend so clients don't have to worry about that.
There was a problem hiding this comment.
Any complexity we can push into the Rust layer means we don't have to duplicate work on each client.
|
Sorry for asking, but shouldn't this feature be a part of the AnkiHub add-on instead of vanilla Anki, since these two aren't exactly maintained by the same people, and may lead to new users confusing them. Secondly, from an outsider's view (who doesn't understand the difference between these similarly named products), this might seem like Anki is officially promoting microtransactions, since Ankihub is essentially a paid service. I'm very thankful for what the Anking team has done for the community, and especially you for making so many add-ons, code contributions and much more. However, I believe significant considerations are still warranted before integrating a third party service into the vanilla ecosystem. |
Of course, none of what I say matter if dae is actually endorsing AnkiHub (but the point about droid app still stands imo) |
|
So, are you suggesting to add integration of a closed source, paid, third-party software to the main app? Really, why? |
|
What are you talking about, Brayan? I don't understand when did I suggest integrating a paid third party software. I'm saying 1. Anki should more prominently mention that this is a not officially endorsed software (if this is in fact the case). 2. Don't prompt Anki users to create an account there in any way. I think the "sign up now" link should be removed. I'm not saying the rest should stay but if dae wants it who am I to go against it. I'm just providing feedback based on the assumption that everyone would like AnkiHub to be differentiated from AnkiWeb because as you said, paid, closed-source, third-party software. |
I'm asking the PR author (@abdnh) the motivation of this PR. Not talking about anyone else's commentaries. Sorry if I wasn't clear enough. |
|
Some more questions to make this clearer:
|
|
I like AnkiHub but I think native support will not offer more than the addon |
|
For some background, the AnkiHub crew and I have spoken about this a bit already. AnkiHub would like to integrate their syncing code into the Rust layer, so that the mobile clients can be used directly with AnkiHub, instead of requiring access to a computer. This PR is an initial step towards that. I was concerned that including this feature would appear to be promoting AnkiHub, and the existing descriptions were at my suggestion, as I want it to be clear to people that it's optional, and costs money. Suggestions on how to make it clearer are welcome. Ideally, the collaboration features would be offered directly by AnkiWeb. But it's likely going to be quite a while until that happens. In the mean time, this is a step towards providing users with AnkiHub syncing across all platforms - at least for those who can afford the monthly fee. |
rslib/src/ankihub/login.rs
Outdated
|
|
||
| #[derive(Serialize, Deserialize, Debug)] | ||
| pub struct LoginRequest { | ||
| // FIXME: we need to pass either `username` or `email` |
There was a problem hiding this comment.
Any complexity we can push into the Rust layer means we don't have to duplicate work on each client.
ftl/core/sync.ftl
Outdated
| @@ -54,6 +54,12 @@ sync-upload-too-large = | |||
| Your collection file is too large to send to AnkiWeb. You can reduce its | |||
| size by removing any unwanted decks (optionally exporting them first), and | |||
| then using Check Database to shrink the file size down. ({ $details }) | |||
| sync-ankihub-dialog-heading = Enable AnkiHub? | |||
| sync-ankihub-username-label = Username or Email: | |||
| sync-ankihub-signup-label = Don't have an AnkiHub account? <a href="{ $signup_link }">Sign up now</a><br><a href="{ $password_reset_link }">Forgot password?</a> | |||
There was a problem hiding this comment.
Please use separate strings instead of one big HTML one, as the mobile clients will likely use native widgets for now, not HTML.
Perhaps the AnkiHub login section on desktop could be added in via their addon instead of being baked in, as a concession. It'd be understandable if the mobile clients did so, as they don't support addons yet
Would this include AnkiDroid? @BrayanDSO is one of its active contributors and some of the concerns he raised above have still not been answered. This might indicate that more discussions are to be had among devs from the entire ecosystem |
My question keeps unanswered.
If it is an open standard that any platform can use, like AnkiCollab or AnkiHub, I support it 100%. If it is an endorsement of a closed, third-party, and paid platform like AnkiHub, I'm strongly against it, despite of how incredible it may be. Since AnkiDroid doesn't have a BDFL governance like Anki, the decision depends on the other maintaners' opinions as well. |
|
Damien, with your gate-keeping control and knowledge of overall architecture, you could require this PR puts in place a nicely thought out interface / provider system such that all similar providers (including open ones) benefit. The arguments for / against are relatively obvious so I won't waste the bits but the difference in future state (short and long-term) are worth considering IMHO, they're pretty different |
|
Please please make this open and make it benefit everyone. Ankihub would benefit a lot from millions of mobile users, so that seems only fair to ask. But please also think if it is good to do this at all. I use Anki because it is open-source and I can get good decks for free. I know this is the same for a lot of people in my country and everywhere. If more decks go to Ankihub what then? So far Ankiweb still has great language decks, but what if that stops? Even if they make Ankihub free one day, it is bad that a closed website by someone else controls all the good decks, no? Also a lot of friends are already confused about what is the real Anki and what is fake. This makes it even more confusing, no? If I know nothing about Anki and look at this, do I understand it? It is strange to have a website to download paid decks next to settings to sync my cards. |
|
I've replied on the forum thread that was created: https://forums.ankiweb.net/t/integrate-an-open-interface-for-collaboration-plaftorms/46372/9
My understanding is that the initial implementation will effectively call ankihubSync() at the end of ankiwebSync(), so there isn't really anything that can be refactored into a reusable component at this stage. The implementation would serve as a reference for future implementors however, and in the future if it needs a UI, we'll need a way for such features to expose a config page. |
|
It's not mentioned yet but many AI features are getting released this year: https://community.ankihub.net/t/march-may-2024-updates/231199 But since many people raised objections about this being a buzzword, we reworded the description to be like this: |
@brishtibheja, I responded to this here. |
Promoting third-party servicesThe recent changes are certainly an improvement. I can understand why some people would (still) see this as promoting AnkiHub, as it may make people aware of the service who were not otherwise. We're not going to fully solve that without making it a hidden option, which I think would be over the top. I think the current service description is not unreasonable, but until we have a free alternative like AnkiCollab in the list as well, I can see how it could be seen as promotional by some. I like the idea of a a 'learn more' link. It keeps promotional text out of the translations and shifts translation responsibility onto the third-party service. It also keeps those strings outside of the codebase, which some of our more puritanical users may appreciate. That said, until AnkiCollab or some other option is in there too, perhaps the approach least likely to upset anyone is to remove the promotional text, and avoid adding a link for now, so only "AnkiHub" is shown. It still unlocks functionality for mobile clients (when this is all implemented), and offers a modicum of discoverablility for now. What do you think? Clarifying future plansThis PR is trickier than normal to review. It's an MVP/first step towards something bigger, and thus there's not a lot of code to review at the moment, but it's also acting as a proxy for the larger question of "is the bigger goal of this and future PRs something that is likely to be merged?" On the one hand, I understand why it has been done this way: AnkiHub doesn't want to invest a lot of time in building something that is unlikely to be accepted, and dividing large changes up into smaller steps can minimize wasted effort. On the other hand, it means I need to a) consider that the current changes are a stepping-stone, and not the final point we want to end up at, and b) need to make a decision based on my understanding about future plans, instead of the code in front of me. The following is how I understand you plan to move forward. Is it correct?
Brayan raised some concerns about the login screen being implemented in Qt, which will mean the mobile clients need to implement their own versions. What are your future plans here? Are you expecting a Svelte prefererences screen to be implemented before we integrate this into the mobile clients? Or are you expecting us to reimplement the login screen for the mobile clients for now? Private approval
To be fair, these were preliminary discussions done over a year ago, with no guarantee of approval. AnkiHub have still had to submit a public PR, which I haven't exactly rushed through, so the community at large has had a chance to participate too. But I take your point - I regret that we didn't do a great job of providing the full context or future plans from the start, which has probably caused some undue alarm. |
Just speaking about my impression here, I was thinking of the current login screen as a placeholder until we do the larger work of syncing integration, with no expectation for it to be implemented for mobile clients at this stage. |
Let's be honest. This isn't not going to benefit AnkiHub. The question is, is that really a problem? I think the important thing is the motivation. The motivation is not promotion. The motivation is to solve serious user experience issues that lead to med students deciding not to use Anki. As we solve problems for our users, our goal is to do the same for all Anki users. This is a step in that direction.
The primary goal is to help people who are starting their journey on AnkiHub. So, while removing the description and link isn't ideal, it's not a major problem. That said, I think having a description and a link makes sense. But I also think it makes sense to move forward with an option that doesn't upset others.
👍🏻
That's fine as long as users can log in again when the token expires. Tokens expire every 4 weeks or if the user logs in on a third device. Of course, this is all configurable on our backend.
During the next step, we can consider how to build something that will support potential future integrations. For example, we could make a log-in screen that shows options for whatever integrations the user has enabled. I am all for building something that others (even free alternatives/competitors) can benefit from. This applies to the "open interface" idea as well.
I agree and am all for more transparency and open discussion with the community, especially since we would like to start considering how we can contribute in a way that benefits all Anki users, not just AnkiHub users. |
|
When designing the architecture, I strongly suggest trying to make it an extension of the app instead of integrating it, by adding an interface that you can use. That way, it reduces the maintenence burden of Anki and AnkiDroid, and a development burden in AnkiMobile, where you supposedly can't help because it is closed source. It would also help your product, since it makes possible to update your extension without having to create a PR here and pass through all the reviewing process. This PR was created almost two months ago and it hasn't moved much, so I think that it serves at least a little as an example. Additionally, it would reduce the odds of breaking the extension when doing something else (e.g. 24.06 and 24.06.1 launches, which had simple and obvious issues that got launched due to a lack of testing). Moreover, you would be able to release updates independently of Anki's release cycle, which commonly takes months to happen. That's probably only possible with Javascript/Typescript, but I don't see any problem with that, as it can call any defined Rust methods. If you don't care about updating it because you expect it to be still and stable, I still reinforce the point about reducing the maintenance burden, since Anki's backlog is relatively big at the time being, and the rhythm here isn't that fast. |
qt/aqt/profiles.py
Outdated
| @@ -708,3 +708,15 @@ def set_network_timeout(self, timeout_secs: int) -> None: | |||
|
|
|||
| def network_timeout(self) -> int: | |||
| return self.profile.get("networkTimeout") or 60 | |||
|
|
|||
| def set_ankihub_token(self, val: str | None) -> None: | |||
| self.profile["ankiHubToken"] = val | |||
There was a problem hiding this comment.
I suggest adding a prefix to the third-party keys. The name spaces will be a mess when other services are added
|
We're not currently planning more changes on this @dae, unless if you think we need to implement this now:
As more third-party code is added, the link idea will no longer be practical of course. A separate Fluent repo for third party translations should be easy to add. Do you think services should also handle infrastructure (e.g. Pontoon) or you're OK with including them in https://i18n.ankiweb.net/? |
|
Sorry guys, this is still on my radar, but got pushed back due to lack of time and lower-hanging fruit. Hopefully should have another review/reply by tomorrow. |
There was a problem hiding this comment.
I think with a few tweaks this should be good to go.
I'm a bit on the fence about the signup link - I fear some users may argue that it's driving traffic to AnkiHub. It doesn't bother me personally, as we have a signup link for AnkiWeb as well, and future services would get the same treatment. But it might be worth cutting out for now as a sign of good faith to that portion of our userbase that cares about this?
Some minor thoughts on the dialog appearance:
- The service description is a bit redundant, as it's already shown in the listing in the preferences screen, but no strong feelings here.
- There's a bit too much whitespace above the signup link, and below the forgot link
ftl/core/preferences.ftl
Outdated
| preferences-ankihub-not-logged-in = Not currently logged in to AnkiHub. | ||
| preferences-ankiweb-intro = AnkiWeb is a free service that lets you keep your flashcard data in sync across your devices, and provides a way to recover the data if your device breaks or is lost. | ||
| preferences-ankihub-intro = AnkiHub provides collaborative deck editing and additional study tools. A paid subscription is required to access certain features. | ||
| preferences-third-party-description = Third-party services are unaffiliated and not endorsed by Anki. Use of these services may require payment. |
There was a problem hiding this comment.
| preferences-third-party-description = Third-party services are unaffiliated and not endorsed by Anki. Use of these services may require payment. | |
| preferences-third-party-description = Third-party services are unaffiliated with and not endorsed by Anki. Use of these services may require payment. |
ftl/core/preferences.ftl
Outdated
| @@ -77,9 +76,16 @@ preferences-network-timeout = Network timeout | |||
| preferences-reset-window-sizes = Reset Window Sizes | |||
| preferences-reset-window-sizes-complete = Window sizes and locations have been reset. | |||
| preferences-shortcut-placeholder = Enter an unused shortcut key, or leave empty to disable. | |||
| preferences-third-party-services = Third-party services | |||
| preferences-ankihub = AnkiHub | |||
There was a problem hiding this comment.
We probably don't need to translate the service name?
There was a problem hiding this comment.
Some blank space above the service name would also be good:
There was a problem hiding this comment.
Some blank space above the service name would also be good:
It looks fine on Windows. Adding some margin doesn't make a difference for me.
ftl/core/sync.ftl
Outdated
| sync-sign-in = Sign in | ||
| sync-sign-up = Sign up | ||
| sync-forgot-password = Forgot password? | ||
| sync-ankihub-dialog-heading = Log into AnkiHub? |
There was a problem hiding this comment.
This would better match other dialogs in Anki:
| sync-ankihub-dialog-heading = Log into AnkiHub? | |
| sync-ankihub-dialog-heading = AnkiHub Login |
I think removing the link for now as a sign of good faith that the intention is not to hijack Anki as a marketing tactic makes sense. Most users will come to this screen from AnkiHub so it's not a big deal.
@BrayanDSO, we made this PR to address some immediate concerns quickly and therefore excluded (temporarily) the important architectural concerns you and others have raised from this PR so that we could treat those considerations more thoroughly after taking this initial step. I know that's not ideal and I apologize for not doing a better job of communicating. For more context, we are rushing to improve the onboarding UX for new students who are starting medical school in the coming weeks. That said, I think it would be great to discuss the next steps, including architecture, and how we can iterate on this in a way that benefits the whole ecosystem, doesn't create unnecessary maintenance burdens, etc, in-depth and openly on the forum. I'll be sure to ping you directly on those topics! |
I don't expect things to get perfect immediately, and I believe that terative improvements is commonly the best way to do things, so please don't be feel rushed. I also don't expect Anki to be able to handle cross-platform extensions for years, given ankitects' current priorities and the repo's pace of development. If you have in mind to avoid as much churn as possible, that's fine to me. From a maintanence point, if you make something that only needs updates in Anki's repository, and not in the other ones, i.e. AnkiMobile and AnkiDroid (besides the initial implementation on them), it's already good enough to me. Doing everything in Rust and Svelte + avoiding changes in the API (like Image Occlusion recently did) makes that possible and isn't complex in terms of architecture. |
qt/aqt/preferences.py
Outdated
| def ankihub_sync_login(self) -> None: | ||
| def on_success(): | ||
| if self.mw.pm.ankihub_token(): | ||
| self.update_login_status() | ||
|
|
||
| ankihub_login(self.mw, on_success, from_prefs_screen=True) | ||
|
|
||
| def ankihub_sync_logout(self) -> None: | ||
| def on_success(): | ||
| self.mw.pm.set_ankihub_token(None) | ||
| self.update_login_status() | ||
|
|
||
| ankihub_logout(self.mw, on_success, self.mw.pm.ankihub_token()) |
There was a problem hiding this comment.
⛏️ ankihub_sync_login doesn't set the credentials (they are set in ankihub_login instead) and ankihub_logout does set the credentials. By setting credentials I mean calling mw.pm.set_ankihub_token and mw.pm.set_ankihub_username. It would be better to make this consistent.
| vbox.addWidget(bb) | ||
|
|
||
| diag.setLayout(vbox) | ||
| diag.show() |
There was a problem hiding this comment.
🔧 When I opened the AnkiHub login dialog, the text was cut off:
| diag.show() | |
| diag.adjustSize() | |
| diag.show() |
AnkiWeb's login dialog has the same problem:
https://github.com/abdnh/anki/blob/1ef83f07f61b178334eb5ce8710648d6bd1c34a3/qt/aqt/sync.py#L339
After adding `diag.adjustSize()`:
|
Thank you for your patience here! |
* Add AnkiHub section to preferences screen * Add short intro for AnkiWeb and AnkiHub to syncing section * Add AnkiHub login screen * Implement login methods in backend * Set minimum dialog width * Add missing colon * Respect the ANKIHUB_APP_URL env var This is used by the add-on. * Simplify login error reporting * Fix from_prefs_screen not passed to subcall * Add missing ankihub_pb2 import * Install AnkiHub add-on after sign-in * Avoid .exec() * Update ftl/core/sync.ftl Co-authored-by: Damien Elmes <dae@users.noreply.github.com> * Split translation string * Support login by username/email * Fix entered username/email not being passed back to on_done * Remove unused import * Move to 'Third-party services' section * Tweak login dialog's heading * Remove 'third-party' from intro text * Tweak copy * Prefix profile keys * Tweak strings * Remove description from login dialog * Remove signup links * Clear credentials in ankihub_logout() * Call .adjustSize() * Title Case * Add padding to third-party services, and fix tab order from other PR
This PR integrates AnkiHub sign-in natively into Anki.
Summary
(The AnkiWeb section was updated to give a short intro about AnkiWeb too.)
@andrewsanchez @RisingOrange