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

Don't display error messages when second scheduled rc command fails #3767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

If we care about errors when using editors like Potlatch,
rc note loading gives an error in Potlatch anyway because Potlatch doesn't have the import command. In case of Potlatch there's no way to check what commands are supported. Potlatch won't even tell that it's Potlatch, unlike JOSM that sends this header: Server: JOSM RemoteControl. And we can't assume that it's Potlatch when there's no server header because we don't get to read the response as long as we make cors-avoiding requests (#3760).

The error message that is displayed when the note import fails is not helpful. It tells the user to check if the editor is launched and rc enabled. But we already know that it's the case because otherwise the first (load_and_zoom) command would have failed. It's better not to display any message at all in this case.

We can't know if load_and_zoom fully worked either. For example, on element pages load_and_zoom gets a parameter to select the element. This parameter is ignored by Potlatch and there's no error message about it. It makes sense then not to display the message about a failed note load.

app/assets/javascripts/index.js Outdated Show resolved Hide resolved
app/assets/javascripts/index.js Outdated Show resolved Hide resolved
@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 4, 2022

Potlatch 3 currently has a "market share" of 0.15% (or 328 users) in 2022 so far. We don't see many users complaining about this issue (or the one mentioned in #3760).

Since this issue is not impacting functionality (you can still edit using remote control), I would label it as a "nice to have" with "low prio". Even closing it would be perfectly fine, I believe.

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.

3 participants