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

[ENH] Remote reporting of unexpected errors #1558

Merged
merged 5 commits into from
Sep 23, 2016

Conversation

kernc
Copy link
Contributor

@kernc kernc commented Sep 15, 2016

No description provided.

@codecov-io
Copy link

codecov-io commented Sep 15, 2016

Current coverage is 88.65% (diff: 100%)

Merging #1558 into master will not change coverage

@@             master      #1558   diff @@
==========================================
  Files            78         78          
  Lines          8121       8121          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           7200       7200          
  Misses          921        921          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 5b5e451...0fca652


# cmd line option overrides settings / no redirect is possible
# under ipython
if options.no_redirect or running_in_ipython():
Copy link
Member

Choose a reason for hiding this comment

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

Does running Orange from ipython shell now work without the need to disable redirects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me and @lanzagar works just as well as on master.


class DockWidget(QDockWidget):
Copy link
Member

Choose a reason for hiding this comment

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

The old name signified that this is a windows that can be docked. The new one suggests that extending classes can only be used as dock components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. Docks that can be docked or undocked but they aren't windows in themselves.

)

self.show_report_action = \
QAction(self.tr("Show Report View"), self,
QAction(self.tr("Show Report Window"), self,
Copy link
Member

Choose a reason for hiding this comment

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

I would rename this option to Report. Show is redundant as this action is located in the View menu and can be considered as View > Report.

QAction(self.tr("Show Output View"), self,
toolTip=self.tr("Show application output."),
triggered=self.show_output_view,
QAction(self.tr("Show Output Dock"), self,
Copy link
Member

Choose a reason for hiding this comment

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

Could this be renamed to Log?

And should we redirect all logs that currently go to the console to this window (i.e. when run with -l 4 flag)?

cache_key = '{}:{}'.format(
frame.tb_frame.f_globals.get('__name__', frame.tb_frame.f_code.co_filename),
frame.tb_lineno)
if cache_key in cls._cache:
Copy link
Member

Choose a reason for hiding this comment

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

I see the point in not showing the send error report dialog if an error occurs multiple times, but some kind of message notifying the user that something went wrong (again) would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


if canvas:
filename = mkstemp(prefix='ows-', suffix='.ows.xml')[1]
# Prevent excepthook prinitng the same exception when
Copy link
Member

Choose a reason for hiding this comment

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

*printing


font.setFamily("Monaco")
font.setPointSize(12)
font.setStyleHint(QFont.Monospace)
Copy link
Member

Choose a reason for hiding this comment

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

Was the error fixed in debian squeeze, or is to old to support even for debian standards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Squeeze is no longer supported. Obviously Squeeze at that time had too old Qt that didn't yet contain QFont.Monospace.

@@ -201,119 +195,39 @@ def __exit__(self, *args):
self.charformat = None


class QueuedCallEvent(QEvent):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not needed anymore?

Copy link
Contributor Author

@kernc kernc Sep 15, 2016

Choose a reason for hiding this comment

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

IIUC it never was needed. If the subscribers to TextStream.stream signal, the only user of this class, prefer queued connections, they can mark them such in their .connect() calls. Otherwise, Qt signal connections are direct if the emitter and handler are in the same thread, queued otherwise.


log.info("Entering main event loop.")
try:
with redirect_stderr(stderr), redirect_stdout(stdout):
Copy link
Member

Choose a reason for hiding this comment

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

The commit message says tee instead of redirecting, while the function called here still says redirect.

REPORT_POST_URL = 'http://qa.orange.biolab.si/error-report/v1/'
ISSUE_TRACKER_URL = 'https://github.com/biolab/orange3'

THUMBSUP_IMAGE = os.path.join(os.path.dirname(__file__), 'thumbsup.png')
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a picture of an Orange orange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


("output/dockable", bool, True, "Allow output window to be docked"),

("help/stay-on-top", bool, True, ""),
Copy link
Member

Choose a reason for hiding this comment

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

Why is the stay-on-top-functionality no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undocked docks stay on top.

STACK_TRACE = 'Stack Trace'

def __init__(self, data):
icon = QApplication.style().standardIcon(QStyle.SP_MessageBoxCritical)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comments timeline doesn't show for it, but the icon has been changed into warning.

@kernc kernc force-pushed the error-reporting branch 5 times, most recently from b07be9d to d996696 Compare September 15, 2016 16:39
@ajdapretnar
Copy link
Contributor

This PR. ❤️

@ajdapretnar
Copy link
Contributor

So far so good on Windows, but is there already a website I could check to see whether my two errors came through?
p.s. The GUI is a bit meh, but this PR is so great I don't even care.

timeout=10,
data=urlencode(data).encode('utf8'))
except Exception as e:
log.error('%s: %s', e.__class__.__name__, e.args[0])
Copy link
Member

Choose a reason for hiding this comment

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

What about
log.exception("Error reporting failed", exc_info=e)



REPORT_POST_URL = 'http://qa.orange.biolab.si/error-report/v1/'
ISSUE_TRACKER_URL = 'https://github.com/biolab/orange3'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used anymore?

@kernc kernc force-pushed the error-reporting branch 2 times, most recently from 5eb51c6 to 10a7729 Compare September 23, 2016 12:34
@lanzagar
Copy link
Contributor

Works and is a much needed feature, so I suggest merging and actively testing in development, before it gets into stable (hopefully soon).

@lanzagar lanzagar merged commit 2bc7ec3 into biolab:master Sep 23, 2016
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.

5 participants