1173984 - [gui] allow to filter log levels

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

A cool feature would be to allow the user to filter some logs, based on their level at runtime.

Comment 1

[MikeLing]

Hi Julien, I would like to work on this :)

Comment 2

[Julien Pagès (:parkouss)]

Eh, cool! Let me know if you have questions :)

Comment 3

[MikeLing]

Attachment: mockup.png

Hi Julien,

I hope this mockup can fit your mind, please tell me if anything should be imporved :)

I just wondering about the exactly way to filter log message. Should we just made those log message we haven't select become invisible?

Comment 4

[MikeLing]

Attachment: patch.patch

Hi Juilen,

Hm, I meet some troubles when I fix this bug. I add a patch and when I run mozregression, it can't work appropriate and shows errors like blow:

> Traceback (most recent call last):
>  File "/home/mike/workspace/opensource/mozregression/gui/mozregui/log_report.py", line 29, in on_log_received
>    self.add_color_to_logView(counter, textColor)
>  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'

I'm not sure if my method is right, please offer some advices about it. Thank you very much :)

Comment 5

[Julien Pagès (:parkouss)]

(In reply to MikeLing from comment #3)
> Created attachment 8624160 [details]
> mockup.png
> 
> Hi Julien,
> 
> I hope this mockup can fit your mind, please tell me if anything should be
> imporved :)

This looks pretty fine to me ! I think it is simple and good, at least for now.

> I just wondering about the exactly way to filter log message. Should we just
> made those log message we haven't select become invisible?

Well I think we should filter using logging levels. That is DEBUG < INFO < WARNING < ERROR < CRITICAL (https://docs.python.org/2/library/logging.html#levels). If a user choose the WARNING level for example, he should see messages that have the level WARNING or CRITICAL. DEBUG would show everything (current behavior).

Also, you have to keep in mind that filtering should not delete the log messages. If a user switch to filter WARNING, then switch back to filter DEBUG (ie no filter), he should see the previous log messages - so you have to keep in memory the list of messages using a list for example.

Comment 6

[Julien Pagès (:parkouss)]

So, I will try to clarify what needs to be done here (at least the way I would do it).


We need to keep the raw log messages (dicts) in memory, for example in a list.

When a new log message is coming, we should add it to the list, but only display it in the text edit if the level is acceptable.

When the user switch to only see some log messages that are under a certain level, we should clear the text edit, loop over our in-memory log messages and display only the ones that are acceptable.


So this suppose that somewhere you have a variable that store the desired level (it could be predefined with a value of DEBUG for example). We should provide an ui to let the user change that desired level (maybe using a menu that would be displayed when right-clicking on the text edit).


MikeLing, does that helps ? Do you need some more clarification ?

Comment 7

[MikeLing]

Yep, I agree with you :)

But my problem is how to make it. Maybe we need a signal to communicate with list and Log View. When a message come to list, it will use a signal to send data to Log View and remind Log View to refresh itself. Furthermore, maybe use a dictionary to store message is better? So we can get specific level message faster.

Comment 8

[MikeLing]

Oops, maybe I found somewhere I was wrong. I will to simplify my thought and retry it.

Comment 9

[MikeLing]

I'm sorry to say that I still don't know how to do this, maybe release it from me and give to other people is a better idea. Thank you for your time and help :)

Comment 10

[Julien Pagès (:parkouss)]

Assigning Kapil because he is interested to work on that bug.

Comment 11

[Julien Pagès (:parkouss)]

Kapil, are you working on this still ? I can help if needed. Also, if it is too difficult we can find other bugs to get started.

Thanks!

Comment 12

[Kapil Singh [:kapy]]

Hey Julien! Sorry for not working on this bug, I was away for a while. Yeah, I may need some help on this bug.

Thanks

Comment 13

[Julien Pagès (:parkouss)]

(In reply to Kapil Singh [:kapy] from comment #12)
> Hey Julien! Sorry for not working on this bug, I was away for a while. Yeah,
> I may need some help on this bug.
> 
> Thanks

Cool, glad you still want to tackle it! So please read the comments in this bugs - that can help - then ask me anything here, or ping me on irc.

Comment 14

[Julien Pagès (:parkouss)]

Kapil, I would like this bug to be fixed relatively soon (say 15 days) because it is blocking bug 1235020. And this is a long standing bug, would be great to see it fixed :)

So, I don't want to stress you (not at all!) but if you are not sure you will have the time, maybe you can take another bug (I can help you find one).

Thanks!

Comment 15

[Julien Pagès (:parkouss)]

Reassigning the bug to nobody - Kapil will look at some other bug later (we had a mail discussion about this).

Comment 16

[Wasif Hyder]

(In reply to Julien Pagès (:parkouss) from comment #6)
> So, I will try to clarify what needs to be done here (at least the way I
> would do it).
> 
> 
> We need to keep the raw log messages (dicts) in memory, for example in a
> list.
> 
> When a new log message is coming, we should add it to the list, but only
> display it in the text edit if the level is acceptable.
> 
> When the user switch to only see some log messages that are under a certain
> level, we should clear the text edit, loop over our in-memory log messages
> and display only the ones that are acceptable.
> 
> 
> So this suppose that somewhere you have a variable that store the desired
> level (it could be predefined with a value of DEBUG for example). We should
> provide an ui to let the user change that desired level (maybe using a menu
> that would be displayed when right-clicking on the text edit).
> 
> 
> MikeLing, does that helps ? Do you need some more clarification ?

Hi,

I've built a list that tracks the incoming log messages. I can filter it based on the debug level needed, however I have a few questions regarding implementation:

1. Should the list be local to the LogView class? (I have currently kept it local)
2. Should I add in a GUI dropdown to select the log levels to display?

Comment 17

[Julien Pagès (:parkouss)]

Hey, great to see that you have made some progress already!

So, for the questions:

1. Hm, thinking more into it, we should not duplicate a list of log entries. I think there is a better way to do this with using the QTextBlocks. Basically each time we add a log line in the document, we add a QTextBlock using the cursor (a QTextBlock is a paragraph, a log line in our case):

https://github.com/mozilla/mozregression/blob/fea2cf216c1aa0293f62009d54405567ad91d8fa/gui/mozregui/log_report.py#L28

You can find the block under the cursor using block(), http://doc.qt.io/qt-4.8/qtextcursor.html#block.

The idea would be to add the log level for each added block, using setUserData, http://doc.qt.io/qt-4.8/qtextblock.html#setUserData.

So each block will hold the information of the log level. Then, to show or hide the blocks for a given level, the idea is to iterate for each blocks of the document and call setVisible, http://doc.qt.io/qt-4.8/qtextblock.html#setVisible.

This way we don't duplicate the whole log list. Does that make sense to you ?

2. Yes, some kind of GUI interface to change the log level value is required. I was thinking of a right click -> level > [list of level] menu in the log view, but I am open to any proposition!

Comment 18

[Wasif Hyder]

Hi,

I've implemented the basic functionality, although there are some troubles I've ran into.

1. I'm using contextMenuPolicy using QtDesigner, then connecting it to a custom menu I created in the LogView constructor. After that, I've attached an action for every filter level ('Debug', 'Info', ...) and connected them to a separate function that filters them.

Problem: For one, the code is very repetitive. I'm looking into a solution that involves using mappers, but its tricky to wrap my head around it. Also, I've created the context menu in the constructor itself, and I'm not sure if that's the right way of doing things. 

2. I can't figure out how to use setUserData. From the documentation, its clear that I have to use QTestUserData class, but its abstract, and I don't know the right way to tackle this. 

My solution: For now, I check every block to see if it contains the word I'm looking for, (Example: "debug") and if that line doesn't, then I filter it out using setVisible(False)

Problem: I don't think this is the neatest solution, but don't know the right way to improve on it. 

What should I do?
(I am going through Mark Summerfield's Rapid GUI Application Development in PyQt, but it'll take me a bit of time and I don't want to halt the progress of this bug since it seems that its blocking something else)

What would you suggest?

Comment 19

[Wasif Hyder]

Here's the gist of the code I've changed.

https://gist.github.com/wasifhyder/c1912d10a7565c51c800

Comment 20

[Julien Pagès (:parkouss)]

(In reply to wasif_hyder from comment #18)
> Hi,
> 
> I've implemented the basic functionality, although there are some troubles
> I've ran into.
> 
> 1. I'm using contextMenuPolicy using QtDesigner, then connecting it to a
> custom menu I created in the LogView constructor. After that, I've attached
> an action for every filter level ('Debug', 'Info', ...) and connected them
> to a separate function that filters them.
> 
> Problem: For one, the code is very repetitive. I'm looking into a solution
> that involves using mappers, but its tricky to wrap my head around it. Also,
> I've created the context menu in the constructor itself, and I'm not sure if
> that's the right way of doing things. 

So yes - you can connect every signals to only one slot, then use the sender() method to get the object that sent the signal.
In you case, some QAction objects are triggered I guess. So you can give an object name to each action ('debug', 'info', ...)
and get the name back in the slot like this:

 lvl_name = self.sender().objectName()

> 2. I can't figure out how to use setUserData. From the documentation, its
> clear that I have to use QTestUserData class, but its abstract, and I don't
> know the right way to tackle this. 
> 
> My solution: For now, I check every block to see if it contains the word I'm
> looking for, (Example: "debug") and if that line doesn't, then I filter it
> out using setVisible(False)
> 
> Problem: I don't think this is the neatest solution, but don't know the
> right way to improve on it. 

Yeah, this is not the best way to do that. So first, to use QTextBlockUserData, I think that you need to create you own subclass. something like:

class TextBlockData(QTextBlockUserData):
    def __init__(self, log_lvl):
        QTextBlockUserData.__init__(self)
        self.log_lvl = log_lvl

Then you can use that to insert in text blocks:

from mozlog.structured_log import log_levels

lvl_name = 'info'  # example, you should read that from the log dict

data = TextBlockData(log_levels[lvl_name.upper()])
text_block.setUserData(data)

Then later to retrieve the log level, this is text_block.userData().log_lvl

> What should I do?
> (I am going through Mark Summerfield's Rapid GUI Application Development in
> PyQt, but it'll take me a bit of time and I don't want to halt the progress
> of this bug since it seems that its blocking something else)
> 
> What would you suggest?


Also, keep in mind that log names are mapped to a number (I called that log level in the above code) defined here: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/structuredlog.py#93

Basically, when the user select the "INFO" level, he should see every level under or equal to the INFO log level, that is "critical", "error", "warning", "info". (only debug should not be visible)

I hope that helps - tell me if something is not clear.

Comment 21

[Wasif Hyder]

Hi,

Just wanted to update you a bit. I might not be able to work on it for two days, although I'm progressing fairly well on the bug. If time is an issue, I'd really like to apolize.

I have exams, so I will be able to work on it after 2 days.

Let me know what you think.

Comment 22

[Julien Pagès (:parkouss)]

This will be fine, now worry. :) Obviously exams first!

Comment 23

[Wasif Hyder]

Hi Julien, sorry for the delay. Exams were hectic.

I've managed to implement the feature as requested. https://github.com/wasifhyder/mozregression/blob/trying-context-menu/gui/mozregui/log_report.py 

Here is the code that I have. There are two more things I wanted to run by you apart from any obvious cases of refactoring that I've missed.

1. How should I go about disabling the actions when there is nothing on the log view docket? Right now, when I click the context menu the actions are appearing as clickable, however when I try it on the other screens the context menu is disable until the regions become active. I've tried looking for a solution but I can't seem to figure it out.

2. Should I add in a button as well, like the one that was originally proposed?

Comment 24

[Julien Pagès (:parkouss)]

(In reply to wasif_hyder from comment #23)
> Hi Julien, sorry for the delay. Exams were hectic.
> 
> I've managed to implement the feature as requested.
> https://github.com/wasifhyder/mozregression/blob/trying-context-menu/gui/
> mozregui/log_report.py 

Excellent !

> Here is the code that I have. There are two more things I wanted to run by
> you apart from any obvious cases of refactoring that I've missed.
> 
> 1. How should I go about disabling the actions when there is nothing on the
> log view docket? Right now, when I click the context menu the actions are
> appearing as clickable, however when I try it on the other screens the
> context menu is disable until the regions become active. I've tried looking
> for a solution but I can't seem to figure it out.

Hm, I am not sure what you mean here. The context menu should only appear in the log view, but this is not a problem if actions are visible when the log view is empty.

> 2. Should I add in a button as well, like the one that was originally
> proposed?

No, the context menu is enough for me. What would be good is to show which level is currently enabled, I think QActionGroup and its exclusive property can allow this (http://doc.qt.io/qt-4.8/qactiongroup.html#exclusive-prop).

I did not tested the code yet, I think there is some conflicts with the mozilla/master branch. Can you rebase your work on top of mozilla/master ?

You can use:

> git pull --rebase https://github.com/mozilla/mozregression

and fix the possible conflicts along the way. :)

to push again to your fork, you will then need to force push:

> git push -f

This is looking promising, thanks Wasif!

Comment 25

[Wasif Hyder]

I've rebased my repository on top of mozregression. 

I'm having trouble with the QActionGroup. What am I supposed to do here exactly? 

My understanding is that I have to show which level is selected, but I don't know how to display that on the context menu.

Comment 26

[Julien Pagès (:parkouss)]

Attachment: qactiongroup example

Here is a working example of using QActiongroup.

the idea is to create a group, register actions in the group, and display the actions. Each action must be checkable (maybe you missed that part ?)

Comment 27

[Wasif Hyder]

Hi Julien,

So an update. All the tests in check are passing, and I've implemented all the features as requested as well as resolving any issues.

The code isn't all too clean at the moment (IMO) and there aren't any tests for it either. Could you kindly advise on how to proceed with testing it? (Before I consider refactoring?)

I'm attaching the link to the latest commit of mine here: 
https://github.com/wasifhyder/mozregression/blob/bugfix-1173984-filter-log-levels/gui/mozregui/log_report.py

Comment 28

[Julien Pagès (:parkouss)]

Hi wasif,

Testing the whole thing may (will ?) be a bit difficult (just a warning :)). If it's too hard/complex, I propose that we just land something working and clean, manually tested. A good compromise might be to test what is easily testable - for example, I don't think that testing the right click menu is easy, but enabling log levels to see if blocks are visible or not probably is.

For the tests, we are mainly using pytest-qt (https://pytest-qt.readthedocs.org), and the test files are in gui/tests. You can look at existing tests to have an idea - best for you is to look at https://github.com/mozilla/mozregression/blob/master/gui/tests/test_log_report.py, I believe you may want to add tests in there.

Once you're ready, please ask me for a review!

Comment 29

[Wasif Hyder]

Attachment: Bugfix -1173984

Comment 30

[Julien Pagès (:parkouss)]

Comment on attachment 8710363 [details] [review]
Bugfix -1173984

Great stuff. There is some nits that I would like to be addressed (github comments), after that, please add a comment here or on github and I'll merge it into master.

Comment 31

[Wasif Hyder]

Attachment: Fixed pull request -Bugfix #1173984

Made the corrections as suggested.

Comment 32

[Julien Pagès (:parkouss)]

Merged in https://github.com/mozilla/mozregression/commit/002dc8221a19998af429337e3242ecb103826dec.

Thanks :)

Wasif, if you are interested, please look at the other bugs! For example, there is some good next bugs:

https://bugzilla.mozilla.org/buglist.cgi?list_id=12807657&resolution=---&status_whiteboard_type=allwordssubstr&query_format=advanced&status_whiteboard=good%20next%20bug&bug_status=NEW&component=mozregression&product=Testing

Have a look - and add a comment if you're interested to work on them.

Comment 33

[Wasif Hyder]

I don't mind tackling them in order. Starting with https://bugzilla.mozilla.org/show_bug.cgi?id=1235003

Exams have ended now - and  I can work on contributing nearly full-time.