Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Jan. 12, 2016, 7:26 p.m. by sergei
Jan. 22, 2016, 12:37 p.m.
Eric, Oleksandr
Felix Dahlke

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


Total messages: 5
Jan. 12, 2016, 7:28 p.m. (2016-01-12 19:28:18 UTC) #1
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 ...
Jan. 13, 2016, 3:14 p.m. (2016-01-13 15:14:13 UTC) #2
It's much better to discuss such things in issue tracker. On 2016/01/13 15:14:13, Eric wrote: ...
Jan. 13, 2016, 4 p.m. (2016-01-13 16:00:57 UTC) #3
On 2016/01/13 16:00:57, sergei wrote: > It's much better to discuss such things in issue ...
Jan. 13, 2016, 4:38 p.m. (2016-01-13 16:38:05 UTC) #4
Jan. 22, 2016, 12:37 p.m. (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

- 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.

Powered by Google App Engine
This is Rietveld