Opened 4 years ago

Last modified 4 years ago

#1237 reopened defect

"The error text was: ?1?" in update error tooltip

Reported by: Gingerbread Man Assignee:
Priority: P3 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: Blocked By:
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4677112304762880/

Description

Environment

Windows XP SP3 • Internet Explorer 8 • Adblock Plus 1.2

How to reproduce

  1. Disconnect from the Internet by disabling your network device in the Network Connections window.
  2. Click the Adblock Plus icon on the status bar and choose Check for Update.

Observed behavior

  • The tooltip title says, "There was an error while checking for an update of Adblock Plus. The error text was: ?1?"
  • The tooltip text is identical.

Expected behavior

  • The tooltip title should be a brief explanation of the information provided, e.g. "Update check error".
  • I have no idea what "The error text was: ?1?" is supposed to be. It sounds like something technical intended for debugging that shouldn't be displayed to users at all.

Attachments (1)

abp_update_error.png (9.5 KB) - added by Gingerbread Man 4 years ago.
abp_update_error.png

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by Gingerbread Man

abp_update_error.png

comment:1 Changed 4 years ago by oleksandr

  • Ready set

comment:2 Changed 4 years ago by eric@…

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

The simple fix for this issue is remove the offending piece of text. The control flow of the current code is not suited for a simple fix to providing the error text, either. And even if it were, it's not a good idea to provide such error text (only) in a tooltip.

libadblockplus provides a non-empty response string; it's the argument to UpdateCallback(). The present issue raises the point that we have no way of reporting that error message back to the user. Perhaps we want another create another issue here for that.

comment:3 Changed 4 years ago by oleksandr

Why isn't it a good idea to provide an error text only in a tooltip? Isn't it better to provide any error text then non at all?
We do have a way of reporting error to the user. We can post WM_UPDATE_CHECK_ERROR with an error message as WPARAM, for example.

comment:4 Changed 4 years ago by oleksandr

  • Cc oleksandr eric@… added

comment:5 Changed 4 years ago by eric@…

  • Cc oleksandr eric@… removed

I do think it would be a good idea to pass along the error text from libadblockplus to the user somehow. But I don't think at all that that a tooltip is the right place for it. It's not going to make immediate sense to most people, which means they're going to need outside help to understand it. And because it's in a tooltip, they can't cut and paste it either. It's just the wrong place for such an error message.

More generally, error messages like this should not appear only in transient UI elements. They can also appear there, if informative enough to ordinary users. In the present case, failure to update is likely an unusual enough error that a useful error report contains detail that most users won't understand. If we had a facility to log to the system event log, this would be the right place to use it.

The present de facto UI, simply a tooltip message that update failed, is a better one that a tooltip message that update failed together with extra detail that most won't understand. The extra data would only create anxiety without enabling the user to do very much about the error, indeed to hinder the user it getting help for the error.

As for WM_UPDATE_CHECK_ERROR, the present ticket arises exactly in that context. We could hack up a fix for this particular ticket, but it just patches over a larger issues, which is that the engine has no general way of asynchronously notifying the plugin of an event. We might use window messages as an underlying mechanism, but we need general mechanisms to deal with type safety and memory allocation. Since we're crossing process memory space, this isn't a trivial matter.

Summary: The best immediate fix is to simply remove the substitution from the present code. The next step would then be to design and implement better error reporting as a whole. That may or may not involve displaying more information in the tooltip.

comment:6 Changed 4 years ago by eric@…

  • Status changed from reviewing to reopened

First review closed as "not fixing in that way".

Note: See TracTickets for help on using tickets.