Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(940)

Issue 29333426: Issue 3501 - use hardcoded value until there is a real error message

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by sergei
Modified:
3 years, 9 months ago
Reviewers:
Eric, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -0 lines) Patch
M src/plugin/PluginClass.cpp View 1 chunk +2 lines, -0 lines 1 comment Download

Messages

Total messages: 5
sergei
3 years, 10 months ago (2016-01-12 19:28:18 UTC) #1
Eric
https://codereview.adblockplus.org/29333426/diff/29333427/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333426/diff/29333427/src/plugin/PluginClass.cpp#newcode1628 src/plugin/PluginClass.cpp:1628: ReplaceString(errorText , L"?1?", L"Unknown error"); I'm uncomfortable with putting ...
3 years, 10 months ago (2016-01-13 15:14:13 UTC) #2
sergei
It's much better to discuss such things in issue tracker. On 2016/01/13 15:14:13, Eric wrote: ...
3 years, 10 months ago (2016-01-13 16:00:57 UTC) #3
Eric
On 2016/01/13 16:00:57, sergei wrote: > It's much better to discuss such things in issue ...
3 years, 10 months ago (2016-01-13 16:38:05 UTC) #4
Oleksandr
3 years, 9 months ago (2016-01-22 12:37:45 UTC) #5
As Eric wrote, our options are:
- Change the error text to avoid substitution entirely.

We may want to display the error text in future, so I don't think we should do
this.

- Use a fixed string
  - Use "". Sergei is against this.

I am against this as well. For example the English message would be:
"There was an error while checking for an update of Adblock Plus. The error text
was: ?1?". In this case "The error text was: _blank_" is more confusing then
undescriptive error text, I think. A user may expect to have undecipherable
error text more then a blank space, I think.

  - Use "Unknown error". I am against this.

- Use the string provided by the ABP core. If it's not localized, it's no worse
than every other version of ABP. This mechanism means passing a string in a
window message across a process boundary.

I don't think there's any argument that this is the best option. This changeset
just fixes the error message until the real error is pulled. 

One more option we have is to use one of the already translated strings. For
example "install-error-text" might work: "An error occurred while updating
Adblock Plus.". However it is no more descriptive then an "Unknown error" in my
optinion, so I'd just stick with that.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5