Fix #153 - Log / Error Notification System

[farazs]

Fix #153. Adding log window to "Help" menu to display log messages. Highest priority log message shown in status bar (latest one only), with an icon to indicate the nature of the message.

• [x] Add menu option to "Help" menu (opens empty window currently)
• [x] Set up log window with list and headings
• [x] Create custom log handler class to populate and add to list
• [x] Add support for filter based on priority (filter visibility only)
• [x] Custom slider for selecting filter level
• [x] Add latest high priority message to status bar
• [x] Add (custom?) icons indicating message nature
• [x] Double-clicking icon or latest log message shows log
• [x] Add calls to translate for log class

[farazs]

The table is now populated and can be cleared. There is no way to filter based on level yet though.

[mtomwing]

A lot of your code seems to be using camelCase instead of PEP8 snake_case.

[farazs]

Yeah I was told by a mentor (I forget who) that UI stuff has an exception and is usually camelCase, whereas everything else is snake_case? So i'm not really sure how that comes into play here. The LogDialog stuff except for the tableModel should be camel and the LogHandler stuff should be snake?

[farazs]

Added filtering based on levels

[farazs]

Maybe the "All" isn't necessary since "Debug" is all the messages anyway. Should I remove "All" or "Debug"?

[farazs]

Btw I'm not too certain about where to use camelCase or snake_case. Should I just use one of them for all the new code? Or where does the distinction between UI and non-UI come in, for eg. the model behind the view is non-UI or UI?

[dideler]

Maybe the "All" isn't necessary since "Debug" is all the messages anyway. Should I remove "All" or "Debug"?

Since looking at a log is a bit more technical, it's probably safe to have "Debug" instead of "All". It's a more accurate representation of the log level.

[dideler]

Btw I'm not too certain about where to use camelCase or snake_case. Should I just use one of them for all the new code?

If there's existing code in the same file, then I think it's important to be consistent with it. For new code, I say use the preferred formatting. Maybe others can weigh in as well.

[dideler]

The table cells shouldn't be editable.

[mtomwing]

In my mind, all new frontend modules should follow PEP8.

[farazs]

As the commit says, I removed the "All" option, removed the grid, made items uneditable, the top header items unclickable and the left header items invisible.

[farazs]

If everyone agrees all PEP8 is the way to go, that would be way simpler since I could have all the names in the module the same style. Or all camel would be good too. Doing half and half in the same file gets kinda confusing.

[farazs]

Added docstrings

[dideler]

If everyone agrees all PEP8 is the way to go

+1

[mtomwing]

It would be useful for new contributors if the originating logger name (e.g. freeseer.framework.plugin vs plugin) was included somewhere.

[farazs]

@mtomwing should I just replace the contents of the current "module" column with the originating logger name?

[mtomwing]

Try it and see how it looks. My only concern with that approach is that deeply nested modules will make the column unreasonably wide.

[farazs]

Yeah that might be an issue. I could put a checkbox called something like "Full module name" that would replace the contents of module with the full name? And have it disabled by default?

Also, should the log level (and the checkbox state if it's added) be saved as preferences so that the user's choices persist?

[mtomwing]

1. I'd like to see how it looks with the logger name first.
2. I'm going to say no as you'll probably only open the full log window when you want to do some debugging and you'll want to see all the log messages (logging.INFO level).

[farazs]

I had to increase the column width for the "module" column since the content is longer. Also, there doesn't seem to be a way to auto-resize a column to fit it's contents so it'll be hard to find a good size. The whole window had to be made bigger as well to account for this larger column. Here's what it looks like:

I don't think this is necessary, since a new contributor can see the source of the log message in the terminal. The purpose was mostly so that users can be notified of errors or messages (in the status bar) and view the log to investigate further.

[farazs]

That was using the pathname attribute of the LogRecord. Here's using the name attribute. It's mostly the same just the '\' replaced with '.' and no ".py" at the end.

[mtomwing]

Looks better to me. Though maybe reduce the "level" column width to fit its content.

I think the dropdown for logging level is a bit misleading too as you might think it filters to JUST that level of log. Maybe it would make more sense to have a linear hierarchy of buttons that make it clear that lower log levels will also be shown?

[dideler]

I like the module column the most in the last screenshot. You can even save some space by removing the repeated "freeseer.".

Though maybe reduce the "level" column width to fit its content.

:+1:

I think the dropdown for logging level is a bit misleading too as you might think it filters to JUST that level of log.

Agreed. Perhaps it's more obvious if the dropdown had options like this

• Debug > Info > Warning > Error > Critical
• Info > Warning > Error > Critical
• Warning > Error > Critical
• Error > Critical
• Critical

[mtomwing]

That might be too much text for a dropdown.

[dideler]

I disagree in the case of this specific dropdown since the dropdown is already enormous and we're giving detailed information in this UI. But I'm all for other ways that are as or more clear and more concise.

Can you elaborate what you mean with a linear hierarchy of buttons? It's possibly a better solution, just not sure how that would look like.

[mtomwing]

Initially I was thinking of:

[Debug] > [Info] > [Warning] > [Error] > [Critical]

A slider might work as well.

[dideler]

I think the linear hierarchy of buttons wouldn't get rid of the confusion, unless I'm missing how it would work in terms of feedback to let the user know which levels are being logged.

But I think a slider could work, as long as the bar fills up.

Like this Not like this

[farazs]

I like the filling slider idea. I will have to look into how to make that possible (if it is possible). As a possible alternative, or incase the slider is not possible, how about just a label saying "Filter Level (including higher levels)" or something like that, that would indicate to the user what's going on in words.

Also, how should I go about determining the size of the columns? Different machines will have different font sizes, and depending on what the user does on the app, the various columns might have content with varying length. What's a good way of determining the initial size (before the user resizes manually)? Should I just leave it how it is right now, a little bigger than needed for these initial messages (with making the level column a bit smaller perhaps)?

[dideler]

"Filter Level (including higher levels)"

I think this would still be confusing because how will the user know which levels are higher (especially if the levels lower in the menu are higher level).

Also, how should I go about determining the size of the columns? Should I just leave it how it is right now, a little bigger than needed for these initial messages (with making the level column a bit smaller perhaps)?

That should be good enough. I wouldn't lose sleep over this, as the user can always resize.

[farazs]

How about a checkbox that says "Show only selected level" which is deselected by default, and selecting it switches the filter to only show the level instead of the range?

[dideler]

That would solve the issue of filtering to a specific level, but I don't think it solves the problem of making it clear what levels will show normally.

[farazs]

Just pushed with docstrings in the correct format and naming in pep8.

Also, pushed the new full module name log with new sizes for the columns and the window:

[farazs]

Switched to using a custom slider for the level selection. I can't seem to find a way to make marks appear on the slider to show where the different values occur. It's supposed to be done using setTickPosition() but they disappear when I use the stylesheet and there doesn't seem to be a stylesheet variable for them.

Here's how it looks:

[mtomwing]

Have you tried setting the stylesheet and then doing setTickPosition(...)?

[farazs]

I hadn't, but I just tried it and it doesn't seem to work either.

[farazs]

Added status icons for status bar (based on last log message)

[farazs]

Double-click to log widgets in status bar now opens the log. Also added calls to translate for the new log class.

[farazs]

Moved status bar log content into it's own widget which is then added by RecordApp to it's status bar.

[farazs]

Tweaked icon size

[mtomwing]

Any reason why the new log widget doesn't also take care of the LogDialog?

[farazs]

Oh do you mean the connect? The new widget could connect its signal to the LogDialog itself?

[mtomwing]

Well yes, but I'm more so concerned with separating things that don't need to know about each other. The LogDialog is only used by the LogStatusWidget right? So why not keep it all inside there?

[farazs]

No, the LogDialog is used by every FreeseerApp. It's the log window that pops up when you go to the "Help" menu and click "View Log".

The LogStatusWidget is the widget in the status bar of only the RecordApp and it uses the RecordApp LogHandler to show the latest high priority message. It also opens RecordApp's LogDialog when it's double-clicked.

Also, how do you make the code boxes around class names when typing here?

[mtomwing]

In that case, I guess it's fine since it can also be triggered from the menu.

All of the Github-flavour markdown can be found here. There's a section just a bit down about code snippets.

[farazs]

I added destructors for the various log classes to remove themselves as listeners from the LogHandler and for the LogHandler to remove itself as a handler from the logger.

I also moved the connection from the LogStatusWidget to the LogDialog inside the constructor of the LogStatusWidget.

[dideler]

Reading the conversation updates, this looks pretty good.

Have you tried getting help in the #pyqt channel about the slider issue? Can even post a link to this PR.

I'll review the code tomorrow since it's getting late here.

[farazs]

It seems it's a bug with Qt:

https://bugreports.qt-project.org/browse/QTBUG-1590?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel

[farazs]

For me the bug happens only for the custom groove. If I remove the groove part of stylesheet, the ticks come back. Looks like we'll have to pick either the ticks or the custom groove. Here's what it looks like:

[dideler]

Hmm that makes it tough. But if we have to choose between one or the other, I think the ticks are more useful. That way you can see how many levels there are and where you have to set the handle. Since it's a slider, it's still a bit more obvious than a dropdown that the lower options (on the left of the slider) are being dropped from the log as you go up.

[farazs]

@dideler did you get a chance to review the code?