Skip to content

Conversation

@tomzx
Copy link

@tomzx tomzx commented Dec 31, 2014

Hi,

This is a small PR that adds the ability for users to specify a custom SYNC_BASE url in the preferences. The default is the https://ankiweb.net/ URL.

Would anyone have a suggestion as how to replicate the functionality I added in AndroidApp.getApplicationPrefs() so it may not be necessary?

This also does not verify if the given URL ends with a slash. It is currently left to the user to format this url properly.

@chajadan
Copy link
Contributor

I just want to add my support that such an improvement be incorporated. Several wanted this and it makes a big difference, and costs only a user preference.

@agrueneberg
Copy link
Member

Thanks for your contribution! Am I correct that anki-sync-server is
currently the only API-compatible alternative to AnkiWeb out there? In
that case, dsnopek/anki-sync-server#16 should
be fixed first before incorporating this. Also, last time I tried,
anki-sync-server had a few issues here and there. I quickly dropped it
because it wouldn't work with AnkiDroid, obviously, so I think both the
server component as well as the Anki ecosystem could benefit from the
inclusion of this patch in the long run. Until anki-sync-server runs
rock-solid, maybe we could find a way to only publish this functionality
in development versions?

I also think that the preference should be in the advanced section
rather than the general one to safeguard users.

On Wed, Dec 31, 2014, at 04:34 PM, tomzx wrote:

Hi,

This is a small PR that adds the ability for users to specify a custom
SYNC_BASE url in the preferences. The default is the https://ankiweb.net/
URL.

Would anyone have a suggestion as how to replicate the functionality I
added in AndroidApp.getApplicationPrefs() so it may not be necessary?

This also does not verify if the given URL ends with a slash. It is
currently left to the user to format this url properly.
You can merge this Pull Request by running:

git pull https://github.com/tomzx/Anki-Android
features/custom-sync-base-url

Or you can view, comment on it, or merge it online at:

#680

-- Commit Summary --

  • Added support for custom Anki Server URL (SYNC_BASE).

-- File Changes --

M AnkiDroid/src/main/java/com/ichi2/anki/AnkiDroidApp.java (4)
M AnkiDroid/src/main/java/com/ichi2/anki/Preferences.java (2)
M AnkiDroid/src/main/java/com/ichi2/libanki/Consts.java (1)
M AnkiDroid/src/main/java/com/ichi2/libanki/sync/FullSyncer.java (3)
M AnkiDroid/src/main/java/com/ichi2/libanki/sync/HttpSyncer.java (8)
M
AnkiDroid/src/main/java/com/ichi2/libanki/sync/RemoteMediaServer.java
(3)
M AnkiDroid/src/main/res/values/10-preferences.xml (2)
M AnkiDroid/src/main/res/xml/preferences.xml (5)

-- Patch Links --

https://github.com/ankidroid/Anki-Android/pull/680.patch
https://github.com/ankidroid/Anki-Android/pull/680.diff


Reply to this email directly or view it on GitHub:
#680

@tomzx
Copy link
Author

tomzx commented Jan 1, 2015

Hi @agrueneberg,

I'm currently in the process of writing another alternative to AnkiWeb in PHP (not yet available) as I'm not comfortable enough fixing anki-sync-server. My end goal is to use AnkiDroid with my alternate service, so I did this change in order to easily change between ankiweb, my local development server and my production server addresses.

I agree with you that since there are not many API-compatible alternatives to AnkiWeb, this preference could go in the advanced section.

Let me know if you want me to move it to the advanced section, and if so, where it would make the most sense.

Thanks for the feedback 😄

@chajadan
Copy link
Contributor

chajadan commented Jan 1, 2015

I agree on advanced section, disagree on wait. It's a simple configurable url -- leaves the world open to do it when and how they want.

@timrae
Copy link
Member

timrae commented Jan 1, 2015

Definitely advanced section. And it's unreasonable to assume the url ends
with a /.... you should add one if necessary in syncURL.

After those changes we can merge into develop for inclusion in 2.5

Copy link
Member

Choose a reason for hiding this comment

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

Please use the constant from Consts.java instead of hard-coding it here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah nice Houssam
On 01/01/2015 12:42 pm, "Houssam Salem" [email protected] wrote:

In AnkiDroid/src/main/java/com/ichi2/libanki/sync/HttpSyncer.java
#680 (diff)
:

@@ -445,9 +444,12 @@ public HttpResponse register(String user, String pw) throws UnknownHttpResponseE
return null;
}

  • public String syncBase() {
  •    return AnkiDroidApp.getApplicationPrefs().getString("syncBaseUrl", "https://ankiweb.net/");
    

Please use the constant from Consts.java instead of hard-coding it here.


Reply to this email directly or view it on GitHub
https://github.com/ankidroid/Anki-Android/pull/680/files#r22397930.

@chajadan
Copy link
Contributor

chajadan commented Jan 1, 2015

For the generic case, it is likely better to leave off an automatic end slash. Not all urls require a slash, it's not a "sync requirement", and urls that end in a querystring never contain a final slash. Most, thought not all urls, that use a final slash don't require the user to supply it (though web server "browse directory" services often require it, and perhaps others). But always adding a slash breaks the generic case.

@timrae
Copy link
Member

timrae commented Jan 1, 2015

@chajadan

it is likely better to leave off an automatic end slash

No slash breaks syncURL()

@chajadan
Copy link
Contributor

chajadan commented Jan 1, 2015

Oh, I see, because the current code forces the appending of "sync/" and
"msync/" onto the base url to accomodate ankiweb's expectations.

On Thu, Jan 1, 2015 at 12:56 AM, Tim Rae [email protected] wrote:

it is likely better to leave off an automatic end slash

No slash breaks syncURL()


Reply to this email directly or view it on GitHub
#680 (comment)
.

@negue
Copy link

negue commented Mar 18, 2015

Is there any chance that this pullrequest gets merged?

@tomzx Great work btw!, but could you resolve the conflicts?

@timrae
Copy link
Member

timrae commented Mar 18, 2015

Yes I'm willing to merge if the conflicts are resolved and the two changes we requested are included. @negue feel free to submit your own PR based on this if there is no response from @tomzx

@tomzx tomzx force-pushed the features/custom-sync-base-url branch from f68ef87 to 3e09e12 Compare March 18, 2015 11:24
@tomzx tomzx force-pushed the features/custom-sync-base-url branch from 3e09e12 to 6b51fed Compare March 18, 2015 11:31
@tomzx
Copy link
Author

tomzx commented Mar 18, 2015

I've moved the Preference in the Advanced section, used Consts.SYNC_BASE in HttpSyncer and rebased my work on the latest changes in branch develop.

@timrae
Copy link
Member

timrae commented Mar 18, 2015

Sorry there were actually three changes requested... the other one being to remove the requirement for the user to terminate the URL with a /.

Also, I had forgotten... the media sync server is now handled by a separate URL (#755), so this won't work anymore - and it currently still has merge conflicts. I'm a bit uncomfortable about having two new preferences just for this though... Maybe you could have a window pop-up in the preferences which lets the user add the two URLs?

@fmap
Copy link

fmap commented Apr 26, 2015

@tomzx Would you be willing to host an APK with this patch somewhere?

@tomzx
Copy link
Author

tomzx commented Apr 26, 2015

@fmap
Copy link

fmap commented Apr 26, 2015

Cheers.

@gniewkos
Copy link

gniewkos commented May 6, 2015

+1 For this PR.

@timrae
Copy link
Member

timrae commented May 30, 2015

I just had an idea; what do people think about having the custom base urls for sync server and media server not be exposed in the AnkiDroid user settings, but settable via the Anki Desktop debug console? E.g. like the number of backups in Anki mobile:

http://ankisrs.net/docs/am-manual.html#automatic-backups

I'd be in favor of including something like that in 2.5

@jshevek
Copy link
Contributor

jshevek commented May 30, 2015

That seems like an unnecessary inconvenience for those that would want to use this setting. Striving for a near-moratorium on new settings is the wrong solution to the problems created by unchecked settings proliferation.

@timrae
Copy link
Member

timrae commented May 30, 2015

You're saying that you think we should add two new settings for a feature
that only a small handful of advanced users will use, and is not supported
by any other anki clients?
On 31/05/2015 1:01 am, "jshevek" [email protected] wrote:

That seems like an unnecessary inconvenience for those that would want to
use this setting. Striving for a near-moratorium on new settings is the
wrong solution to the problems created by unchecked settings proliferation.


Reply to this email directly or view it on GitHub
#680 (comment)
.

@unode
Copy link

unode commented May 30, 2015

Hi all,

I've been following this conversation since almost day one and I agree with @jshevek on this. If there's more than one user asking for this feature, I do not understand why such a reluctance in adding it?
Is it such a maintenance or interface burden that almost half a year since the initial pull request we are still discussing alternatives?

Would be possible to simply accept the request and rework it in the future if deemed necessary?
The issue of the slash at the end seems like something users can live with. There's always the possibility of documentation, and if we are talking "advanced users" I'm sure they will know where to look.

Thanks

@ospalh
Copy link
Contributor

ospalh commented May 30, 2015

more than one user

With me here are now 11 participants with github accounts in this discussion. Those two people that want this can build their own version. That would be simple.

@hssm
Copy link
Member

hssm commented May 30, 2015

Personally I don't like the feature because I don't want to deal with the support requests that result from misbehaving custom servers. However, the most starred issue on our issue tracker is for this feature, so I'm not against putting in a setting for it based just on that alone.

Regardless of what I think, this feature is not going anywhere because as I understand it there are currently no custom Anki servers that are compatible with the current sync protocol. That is, fixing this PR and exposing the setting would achieve nothing because there are no 3rd party services that can currently sync.

@RunasSudo
Copy link

Was there not https://github.com/dsnopek/anki-sync-server as a compatible substitute for AnkiWeb?

@jshevek
Copy link
Contributor

jshevek commented May 30, 2015

You're saying that you think we should add two new settings for a feature
that only a small handful of advanced users will use, and is not supported
by any other anki clients?

@timrae Yes. I think that keeping the most often viewed preference screens simple for casual and new users is important, and that's why I'd support burying rarely used preferences like this. But I don't see any valid reasons related to 'protecting against preference proliferation' to bury them so far that they aren't even accessible within the app, requiring the debug console route.

@hssm
Copy link
Member

hssm commented May 30, 2015

@RunasSudo Please read the third and fourth posts on this PR. The service exists but is broken for any recent version of Anki (and by extension AnkiDroid).

@ghost
Copy link

ghost commented May 31, 2015

@timrae
What do you think of the way the advanced settings section in BeyondPod is designed in terms of clutter, discoverability? Would you be against something like that on a design principles basis? This is more a general question and not specific to custom sync. I find the BeyondPod design pretty much ensures people don't stumble upon those settings unless they are looking for them.

From a support request standpoint would naming such a section "developer settings" help? Perhaps a message basically saying "by entering these settings, you're on your own"?

As far as workload and code maintenance, I have absolutely nothing to suggest often than to thank those who made and maintain this app. It's been extremely useful to me.

@timrae
Copy link
Member

timrae commented May 31, 2015

@unode
As previously mentioned, there are merge conflicts due to #755. I.e. this PR is currently broken, and most likely requires the addition of a second extra preference for the media sync URL. We will merge the PR when the problems are fixed and a consensus to do so has been reached. In the meantime you can easily build your own version of AnkiDroid, it's not very difficult.

@GivHug
Thanks, I just checked out that app, it seems to be designed really well! Accessing the advanced settings via the overflow menu is a nice idea, I like it!

@hssm
I agree, although as @chajadan mentioned earlier, supporting the feature may give some incentive to developers to fix the personal sync server code...

@everyone
What do you think about having unexposed settings set via the debug console for now, and revisiting how we expose these settings at a later point when there is a working solution available?

@timrae
Copy link
Member

timrae commented May 31, 2015

Alternatively we could just tell users to build a custom version, as @ospalh said... I'd probably be willing to write a tutorial on how to do this

@jshevek
Copy link
Contributor

jshevek commented May 31, 2015

What do you think about having unexposed settings set via the debug console for now, and revisiting how we expose these settings at a later point when there is a working solution available?

In this case, because of the "support requests that result from misbehaving custom servers" that @hssm mentioned, I see an additional benefit to explicitly making this setting hard for naive users to access.

But I see this discussion through the lens of 'how should the project handle rarely used settings in general?'. I think there is a wider need for an in-app method of accommodating a range of less common settings. Too many suggestions made on the mailing list have been needlessly shot down on the basis of 'settings proliferation is bad'. Imo, this project needs a place to put new settings without raising these concerns.

I like @GivHug 's thoughts. The 'developer' section could also go under the 'advanced' section. It could require clicking through a warning dialog.

It could even be hidden easter-egg style, requiring an overtly counter-intuitive action to access (like the way phones are put into developer mode by clicking the build number a bunch of times).

@timrae
Copy link
Member

timrae commented May 31, 2015

Sure, I think that a developer settings or similar section in the
preferences, which is made hard to access by one of the methods discussed
so far could be useful! That won't change that each new setting needs to be
well justified though; this is a fundamental design guideline from Google.

Making a decision on the most appropriate way of doing something is usually
better than making a setting for it, and not having a setting to change the
behavior must usually actually cause harm to some users in order to be
accepted.
On 1/06/2015 4:32 am, "jshevek" [email protected] wrote:

What do you think about having unexposed settings set via the debug
console for now, and revisiting how we expose these settings at a later
point when there is a working solution available?

In this case, because of the "support requests that result from
misbehaving custom servers" that @hssm https://github.com/hssm
mentioned, I see an additional benefit to explicitly making this setting
hard for naive users to access.

But I see this discussion through the lens of 'how should the project
handle rarely used settings in general?'. I think there is a wider need for
an in-app method of accommodating a range of less common settings. Too many
suggestions made on the mailing list have been needlessly shot down on the
basis of 'settings proliferation is bad'. Imo, this project needs a place
to put new settings without raising these concerns.

I like @GivHug https://github.com/givhug 's thoughts. The 'developer'
section could also go under the 'advanced' section. It could require
clicking through a warning dialog.

It could even be hidden easter-egg style, requiring an overtly
counter-intuitive action to access (like the way phones are put into
developer mode by clicking the build number a bunch of times).


Reply to this email directly or view it on GitHub
#680 (comment)
.

@timrae
Copy link
Member

timrae commented May 31, 2015

By the way, @tomzx is already hosting a custom build with this patch included (linked to previously)... is anyone using it?

@timrae timrae force-pushed the develop branch 2 times, most recently from b18764b to 12b1c86 Compare July 28, 2015 08:47
@dper
Copy link

dper commented Nov 11, 2015

One drawback to not enabling this feature is that this app is pushing users towards one specific service. At the moment, the main AnkiWeb server is free and relatively stable, but it's also single-point failure. Nobody is developing a working stable server, so nobody is developing a client that can easily be used with one, but clients are hard to use with different servers, so ... we're stuck in this unstable situation, hoping that AnkiWeb doesn't go down.

@timrae
Copy link
Member

timrae commented Dec 30, 2015

Superseded by #4026

@timrae timrae closed this Dec 30, 2015
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.