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
Logging improvements #5176
Logging improvements #5176
Conversation
c283c1d
to
e9dbf54
Compare
I've updated this branch with the latest of my CTK updates if anyone would like to test out the new behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it works really well 👍
Stylistically, I don't really need the filter buttons to take up so much space and the icons are distracting. Plus they aren't intuitive to me that they are toggled on by default.
I would vote for being in Console mode by default and having a small checkbox for Advanced like we do in the DICOM module. (If possible in the title bar next to the Error Log text to save space).
Also, the error colors don't match between the python console and the error; can you use the Bright Text color from the palette for readability in Dark mode?
See discussion here: fe6b5e3
Do you think for regular users, the console mode method should be the default? To me the console mode is the more "Advanced" mode and therefore shouldn't be the default. I could however add a user setting that you could use to remain in console mode for subsequent sessions even though it wouldn't be the default. Regarding the advanced checkbox, are you suggesting to use that to hide all the filter buttons? I actually think the filter buttons are helpful for regular users as they can filter out to see just the errors/warnings which is really all that they care about. Thereby being able to show just the errors or both errors and warnings would be helpful for a regular user. Regarding the text color, the current error text color is pure red which is why it is different from the python console. The color is currently being defined in the ctkErrorLogWidget, but should still have access to get from the palette to use the BrightText specification for errors. What do you think about the orange color used for warnings? That doesn't correspond to any palette entry. |
I think simpler is better for the default. We're often guilty of offering too many buttons and these are particularly off-putting to me because of the error and warning icons. Perhaps I guess I've already said this, but I've never found a reason to use those filter buttons in practice, maybe because different libraries have different conventions and important errors may get printed just to stdout. But yes, maybe putting them in a collapsable frame with the open/close and console mode settings being persisted would be enough. My personal preference would be console mode with buttons hidden. I like the orange a lot, it would be nice if there were a spot for it in the palette. It's the pure red on the dark background I have trouble with. Sonia and I both thought so so I went for the more pink look for the error messages. If you think there's a better variant we could change it for both, but whatever we do I think they should match. |
Can you explain more about this point? This would seem to indicate your preference to hide the buttons is primarily about the icons and not related to space concerns. Is there also an off-putting issue with the error log toolbutton in the status bar? It also shows the info, warning and error icon based on the state. Would having no icon for these buttons be better? Is it just difficult to understand if the button is in the checked state vs unchecked state in dark mode which would be something for all checkable buttons in dark mode? I found the filters helpful especially during a long session where the errors and warnings are not necessarily at the end of the log. Then you don't have to scroll through the entire log searching for all the errors and you can just filter them. |
I agree that console mode should be the default. For developers, the advantages are obvious. For users, it makes it is easier to copy all or most recent error messages. I would prefer to have filters easily accessible. I agree that filters are not used that often but Clear button and auto-scroll toggle button would be needed frequently anyway and if we show those buttons in a row then then there should be space there for the filter buttons, too. We don't have a consistent strategy in Slicer about who are our target audience and design the GUI accordingly, but at some point we should spend some time with this. Maybe we could define a few roles and levels of expertise and decide about what is shown by default accordingly. I'm not hopeful that we can come up with a very good solution (seeing how all other feature-rich software struggles with this, despite tremendous efforts) but we should be able to improve many things. |
Yes, my concern is that the row of buttons looks too much like they are indicators of actual errors and warnings instead of filter modes. But also it's the space issue since they do consume space and I'd rather see more log text. Maybe removing the icons from the buttons would also be good, but just being able to easily hide or show the button row would be enough I think.
Yes, I never cared for the red X for the error log, because even disabled it looked like something was broken. I like that the icon changes based on the state of the log. Having no icon would be okay, but having the info icon is also fine. |
Would it be better if the button text was changed to "Show Errors", "Show Warnings" and "Show Messages"?
Yeah I think I would've liked a red triangle more so but it does appear that a red X in a circle is the more common icon for displaying error messages such as in Windows log viewer. If the error log toolbutton is the red X icon that should actually indicate something was broken now. I did find the red X in the disabled look as it is currently in the Slicer code to appear odd and broken since the button was truly disabled and was confusing to know if there was actually errors or not. That's why I've changed that toolbutton behavior here in this PR. |
Thinking more about this, yes, I think a row like "Filters: [x] Errors [x] Warnings [x] Messages" would be more intuitive and just as practical. We could make it a configurable option in CTK so that the default could be as-is.
Agreed, the new mode is much better. |
It is hard to reconstruct from the message what is the current plan. Could you attach a mockup or screenshot to illustrate? I've realized that use the red status icon and filtering a lot: when I see that the status icon is red then I click it. Most of the time there are info messages logged before and after the error, so I uncheck info level to see the error message. If it is something harmless then I click Clear and close the window. I do this whenever I notice that the status icon is red. The changes should not increase the number of clicks necessary to achieve this. |
I think what @pieper was suggesting was the use of checkboxes like the following. I've detailed that here with and without icons showing. Also showing with the palette defined red text color |
I think the icons with the checkboxes is better than just the checkboxes. The icons provide a relationship with the icon of the error log toolbutton icon that then provides a textual understanding of what each icon means. So the user learns yellow triangle means warnings when there is a icon shown with the checkbox. I was also playing around with more vertical lines between the filters in this manner since it wasn't super clear if the checkbox was for the text/icon to the left of it or the text/icon to the right of it. |
The checkboxes really clarify the intent to me, thanks for playing with that. I agree that including the icons is nice. Could they be made smaller? That would save space but also make them less attention grabbing. |
Thank you, these are excellent examples and we can learn a lot from them. Visual Studio: Has real console and table views, separately. I know form lots of first-hand experience that both have serious limitations. I need to keep both visible because I can see more messages in console view and in correct order, but impossible to find errors/warnings in a long output. Chrome, Firefox: No table view is available, it is just a very nicely formatted console view. It is also interesting that it is not just a log view but also a command console. Main conclusion: I much prefer chrome/firefox approach, as it combines 3 functions into 1: command console, log view in table format, log view in console format. In the short term, we should try to make console-mode log view formatting nicer and completely drop the table format. In the mid/long term, we should try to integrate with command console. Minor takeaways:
|
Nice discussion on improvements to how we can view logging. Can you all provide me guidance with what is necessary to complete this specific PR? And what can get put into a new Slicer issue for detailing extra improvements for whoever has the additional time to complete? |
Again, great discussion! For what it's worth, my opinion on the main points touched upon above:
|
We can use ctk flow layout, which automatically switches to breaks the button row into multiple rows if column is too narrow. |
Could you try looking into using nicer formatting in the Python console? If we can display log messages nicely in the Python console (and implement filtering) then we could skip one intermediate step and jump to the final solution: leave the log viewer as is (don't use it in Slicer at all) and just redirect all logs to the Python console (we would just call it "Console", not Python console anymore). If it's too much to do in one step then I would suggest to update ctkErrorLogWidget to use a console and nice formatting and drop the table mode to keep things simpler. If there is anyone still interested in the table mode then we can create a new class instad (something like "ctkConsoleLogWidget") and leave ctkErrorLogWidget as is. |
This makes the Error Log like the python interactor where the toolbar button shows/hides the dock widget. It is also by default displayed in the bottom area like the python console.
This is set up similar to the application pythonConsole reference.
$ git shortlog 92d2191..7cda431 James Butler (5): ENH: Add console mode button for ctkErrorLogWidget ENH: Add color to Error and Warning description text in ctkErrorLogModel ENH: Scroll Error log description to bottom when table view hidden and scroll on updates ENH: Add properties to set visibility for all buttons in ctkErrorLogWidget ENH: Hide the "All" entries button in the ctkErrorLogWidget to minimize width Jean-Christophe Fillion-Robin (1): COMP: Fix build of ctkVTKWidgetsUtilsTestImageConversion using Qt 5.10
Python INFO, WARNING, and ERROR messages generated by the python logging module created duplicate entries as seen in the Slicer log file and in the Error Log Dialog. This commit removes python logging messages from showing in the python console as they are already included in the Slicer log and should be viewed there.
e9dbf54
to
1570ff7
Compare
#5156 brings significant improvements into logging in the Python console. It might even serve as a replacement for the new console mode. Let's spend some time gathering experience with the Python console and then revisit this pull request. |
Included in this PR is a commit to make the error log a dockable widget. That functionality was ultimately integrated as part of 5d069c7. |
This would supersede #5156 and the changes proposed here are based on discussions from that PR.
Requires changes in commontk/CTK#928 where there are changes such as:
a console mode button which allows viewing the log not in table format, but just simple text format. This is supported by selecting all fields when hiding the table. When hiding the table you can still use the filter buttons to view all errors in the simple text format as well.
supporting colored text in the description column and full description fields. Warning (orange), Error and Critical (red)
The ctkErrorLogWidget table view option is shown by default instead of the "Console Mode" option being in the checked state.
The log text in the ErrorReport Dialog area is no longer editable. I've made it read-only as I don't think it was intended to be editable there.See 7e6a62d