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

Improve logging code and display #143

Merged
merged 10 commits into from Jul 4, 2013
Merged

Improve logging code and display #143

merged 10 commits into from Jul 4, 2013

Conversation

@zas
Copy link
Collaborator

zas commented Jul 3, 2013

  • Limit size of stored lines in memory (log.entries became a collections.deque with maxlen = 50000)
  • Ensure log view dialog receiver is removed on dialog close
  • Use numeric log levels instead of string prefix, easier to work with

capture du 2013-07-03 14 56 06
capture du 2013-07-03 14 56 12

self.textCursor.movePosition(QtGui.QTextCursor.End)
self.textCursor.insertText(prefix + ' ' + str(QtCore.QThread.currentThreadId()) + ' ' + time + ' ' + msg, self.textFormat)
formats = {

This comment has been minimized.

Copy link
@mineo

mineo Jul 3, 2013

Member

This dict never changes but is recreated every time this function is called, it should be a module-/class-level constant instead.


_entries = []
_receivers = [_stderr_receiver]
_receivers = {}

This comment has been minimized.

Copy link
@mineo

mineo Jul 3, 2013

Member

I'm not sure a dict is necessary here because the only key-based access is in unregister_receiver (well, and register_...) but that can be done with list.remove just as easily.



def _stderr_receiver(level, time, msg):
prefixes = {

This comment has been minimized.

Copy link
@mineo

mineo Jul 3, 2013

Member

This is constant as well.

self.textFormatDebug.setForeground(QtGui.QColor('purple'))
self.textFormatWarning = QtGui.QTextCharFormat()
self.textFormatWarning.setFont(font)
self.textFormatWarning.setForeground(QtGui.QColor('orange'))

This comment has been minimized.

Copy link
@mineo

mineo Jul 3, 2013

Member

Hm, I do have to admit that the orange text on white background is hard to read, especially with all other entries having relatively dark colors. I just tried it with 'darkorange' which already improves this a lot.

This comment has been minimized.

Copy link
@zas

zas Jul 3, 2013

Author Collaborator

Agreed ;)

@zas

This comment has been minimized.

Copy link
Collaborator Author

zas commented Jul 3, 2013

capture du 2013-07-03 18 45 29


def _stderr_receiver(level, time, msg):
sys.stderr.write("%s: %s %s %s%s" % (_log_prefixes[level],
str(QtCore.QThread.currentThreadId()),

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2013

Member

I noticed you removed the currentThreadId from the log window receiver, but not from this one. We should be consistent in how things are displayed. (There's the question of how useful this is since it's apparently the thread the receiver is executing in, not where log.whatever() was called from, but might be safer to just keep it.)

This comment has been minimized.

Copy link
@zas

zas Jul 4, 2013

Author Collaborator

I don't know if currentThreadId is useful or not (as is), but if it is i don't think Picard users need it (devs may though). This is why i removed it from log view.
About consistency, i generally agree but here it was intentional:

  • GUI log window for everyday's user (no search)
  • console for experienced user and dev (search/grep etc).

I have in the idea to display history of status bar messages in similar fashion (because those are useful but sometimes just disappear too fast, or are too many).

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jul 4, 2013

Member

The only thing I've ever used the log window for is obtaining tracebacks from users without them having to run Picard from a terminal. In general, non-developers aren't using these messages for anything except reporting bugs. So the messages still have to be useful for developers, because they're the ones fixing the bugs. I don't think we should appropriate this dialog for everyday users.

The status bar idea sounds interesting, but it should be something entirely separate from the log window (and more visible - in my experience, most users on the forum don't know that the log window exists, let alone what it's for).

Anyway, what I wrote above is moot here. Since currentThreadId doesn't appear to be useful, I would just remove it from the stderr receiver as well.

This comment has been minimized.

Copy link
@zas

zas Jul 4, 2013

Author Collaborator

Done in zas@bb8176b

@zas

This comment has been minimized.

Copy link
Collaborator Author

zas commented Jul 4, 2013

Squashed few commits and rebased on current master.

mwiencek added a commit that referenced this pull request Jul 4, 2013
Improve logging code and display
@mwiencek mwiencek merged commit 72dce65 into metabrainz:master Jul 4, 2013
@zas zas deleted the zas:logview branch Jul 4, 2013
@mineo

This comment has been minimized.

Copy link
Member

mineo commented on be7965d Jul 8, 2013

Bisecting shows that this commit breaks most of the format tests:


Traceback (most recent call last):
File "/home/wieland/dev/picard/test/test_formats.py", line 77, in test_simple_tags
loaded_metadata = save_and_load_metadata(self.filename, metadata)
File "/home/wieland/dev/picard/test/test_formats.py", line 44, in save_and_load_metadata
loaded_metadata = f._load(filename)
File "/home/wieland/dev/picard/picard/formats/apev2.py", line 54, in _load
log.debug("Loading file %r", filename)
File "/home/wieland/dev/picard/picard/log.py", line 66, in debug
thread.proxy_to_main(_message, LOG_DEBUG, message, args, kwargs)
File "/home/wieland/dev/picard/picard/util/thread.py", line 110, in proxy_to_main
ThreadPool.instance.call_from_thread(handler, _args, *_kwargs)
AttributeError: 'NoneType' object has no attribute 'call_from_thread'

This comment has been minimized.

Copy link
Member

mwiencek replied Jul 8, 2013

Indeed, that's one of the reasons I submitted #146, which fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.