1173981 - [gui] add colors for the log levels

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

Once bug 1170093 will be landed, we will have working a log view. Let's add some color!

I propose to color only the level name in the log messages. Also some colors will be added with bug 1165377 to the report view rows, we should pay attention to be consistent with the colors.

Comment 1

[Julien Pagès (:parkouss)]

MikeLing, are you interested to work on this after bug 1170093 ?

Comment 2

[MikeLing]

Sure! I will take it once bug1170093 been merged.

Comment 3

[MikeLing]

Hm, I just wondering are there any other kind of 'level' message in log message? All I got is 'DEBUG' for now :(

Comment 4

[Julien Pagès (:parkouss)]

hm, yeah may have info and warning levels also I think. you can see some log calls by running:

git grep 'logger\.info'
git grep 'logger\.warning'

Comment 5

[MikeLing]

Attachment: patch.patch

The detail information could be found in here(https://bugzilla.mozilla.org/show_bug.cgi?id=1173984#c4)

I'm so sorry about I comment in wrong place. Sorry! :(

Comment 6

[Julien Pagès (:parkouss)]

I can see this line in your patch (did not tested it):

textColor = QBrush().setColor(color[data['level']])

I'm pretty sure that setColor() just set the color an return None - so your textColor variable is None and it crash because of that, and this pretty explicit with the stack trace:

>  File "/home/mike/workspace/opensource/mozregression/gui/mozregui/log_report.py", line 17, in add_color_to_logView
>    fmt.setBackground(textColor)
>TypeError: QTextFormat.setBackground(QBrush): argument 1 has unexpected type >'NoneType'

Comment 7

[MikeLing]

Attachment: patch.patch

Still doesn't work for this version patch. No colors show up :(

Comment 8

[Julien Pagès (:parkouss)]

The problem is that you are trying to change the color of the line after you inserted it. It does not work that way, you should instead use a QTextCursor to add some text at a lower level (instead of the QPlainTextEdit::appendPlainText method) - it will allow you to add some colour.

Please look carefully at the documentation:

http://doc.qt.io/qt-4.8/qplaintextedit.html
http://doc.qt.io/qt-4.8/qtextcursor.html

And you should be able to understand how that works and implement what is needed here. Maybe looking at the Qt examples, and looking for help on google may help also if you are stuck.

Take your time, and tell me how it goes!

Comment 9

[MikeLing]

(In reply to Julien Pagès from comment #8)
> The problem is that you are trying to change the color of the line after you
> inserted it. It does not work that way, you should instead use a QTextCursor
> to add some text at a lower level (instead of the
> QPlainTextEdit::appendPlainText method) - it will allow you to add some
> colour.

Hi Julien,

The problem is I haven't use appenPlainText and I insert text and set background color at the same time.

> cursor_to_addMessage.insertText(log_message + "\n", fmt)

Comment 10

[MikeLing]

Attachment: PR of Bugfix1173981

After second thought, I think we should keep the way of log message show. I test it locally and the message do automatically shows up and let user see the latest message. So I decide to keep it rather than file a new bug.  What's your opinion?

And I will add a new PR to make the code more readable(use setMaximumBlockCount instead of count the number of blocks)

Comment 11

[Julien Pagès (:parkouss)]

(In reply to MikeLing from comment #10)
> Created attachment 8628083 [details] [review]
> PR of Bugfix1173981
> 
> After second thought, I think we should keep the way of log message show. I
> test it locally and the message do automatically shows up and let user see
> the latest message. So I decide to keep it rather than file a new bug. 
> What's your opinion?

Sounds good to me. :)

> And I will add a new PR to make the code more readable(use
> setMaximumBlockCount instead of count the number of blocks)

Sure, setMaximumBlockCount seems like exactly what we wanted! we will remove the code that remove blocks by the way. Excellent MikeLing! Yes, please do a PR for that. I don't think we need a bug (but feel free to create one if you want).

Comment 12

[Julien Pagès (:parkouss)]

Comment on attachment 8628083 [details] [review]
PR of Bugfix1173981

Looks pretty good to me, thanks!

I noticed a few things I would like to be changed on the github PR. Once you addressed them, please tell me and I'll be happy to merge this in. :)

Comment 13

[Julien Pagès (:parkouss)]

Thanks!

Merged in: https://github.com/mozilla/mozregression/commit/bd40f3d7a84b37fe4c2b46e44170db29686c0620