GNOME Bugzilla – Bug 709294
no event compression used
Last modified: 2017-07-24 11:22:12 UTC
When there are a number of duplicate events in a row we should try to compress them into one entry and indicate how many of them there are.
Perhaps we can indicate the number of duplicate events with a counter next to the message: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/logs/logs-mixed-fonts.png
I'm coming back to this bug because I think it's probably the main improvement that's needed to make Logs really useful. Do let me know if any extra design work is needed, such as how to present collapsed events when they're opened.
(In reply to Allan Day from comment #2) > I'm coming back to this bug because I think it's probably the main > improvement that's needed to make Logs really useful. Do let me know if any > extra design work is needed, such as how to present collapsed events when > they're opened. I have a doubt about how exactly to group the duplicate events ? like we can group them as duplicate if they refer to exact same message or if they refer to messages with some given percentage of similarity (we can use similar string detecting algorithms like levenshtein distance and other for this purpose). If grouped as duplicate events which refer to exact same message, while showing the collapsed events, I presume, we can just show the detailed view of event as we show right now. But if we use some given percentage of similarity as grouping criteria, then we might need a different way to showcase the collapsed events. Perhaps, maybe we can show the collapsed events in another GtkListBox in the detailed view. After this design part is clarified, I can proceed on to the implementation.
Created attachment 351268 [details] Screenshot showing duplicate message types I have attached a screenshot of the image showing the two types of duplicate messages mentioned by me in the above comment.
@Allan Could you please take a look at comment#3 if you have some time? Some design work needs to be done. Thanks.
(In reply to Pranav Ganorkar from comment #3) > > I have a doubt about how exactly to group the duplicate events ? like we can > group them as duplicate if they refer to exact same message or if they refer > to messages with some given percentage of similarity (we can use similar > string detecting algorithms like levenshtein distance and other for this > purpose). > > If grouped as duplicate events which refer to exact same message, while > showing the collapsed events, I presume, we can just show the detailed view > of event as we show right now. > > But if we use some given percentage of similarity as grouping criteria, then > we might need a different way to showcase the collapsed events. Perhaps, > maybe we can show the collapsed events in another GtkListBox in the detailed > view. > I agree with this solution. One thing I want to clarify is that we only want to compress log entries that are close to each other.
(In reply to Jonathan Kang from comment #5) > @Allan > > Could you please take a look at comment#3 if you have some time? Some design > work needs to be done. Exciting to see this being worked on! Event compression could really make the difference for logs! About which logs to compress - I think this will probably need some experimentation. Certainly we could think about collapsing: * Identical messages * Messages from the same sender that are close together in time * Messages that have the same beginning (identical first word, possibly) Here's a mockup for how it could look: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/logs/logs-message-compression.png
(In reply to Allan Day from comment #7) > (In reply to Jonathan Kang from comment #5) > > @Allan > > > > Could you please take a look at comment#3 if you have some time? Some design > > work needs to be done. > > Exciting to see this being worked on! Event compression could really make > the difference for logs! > > About which logs to compress - I think this will probably need some > experimentation. Certainly we could think about collapsing: > > * Identical messages > * Messages from the same sender that are close together in time > * Messages that have the same beginning (identical first word, possibly) > > Here's a mockup for how it could look: > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > logs/logs-message-compression.png The mockup looks pretty good. But would it be feasible to display large messages inside the popover as opposed to the currently implemented detailed entry view. Perhaps, we will need to ellipsize such messages so that they fit nicely in the popover. To keep things simple initially, maybe we can compress logs which have identical messages and messages that have same beginning word. Let me know your thoughts regarding this.
(In reply to Pranav Ganorkar from comment #8) > > The mockup looks pretty good. But would it be feasible to display large > messages inside the popover as opposed to the currently implemented detailed > entry view. Perhaps, we will need to ellipsize such messages so that they > fit nicely in the popover. Currently a tooltip is showed when you hover your mouse on a log entry. So my idea is we can ellipsize these super long messages and show a tooltip for the full message.
(In reply to Pranav Ganorkar from comment #8) ... > The mockup looks pretty good. But would it be feasible to display large > messages inside the popover as opposed to the currently implemented detailed > entry view. Perhaps, we will need to ellipsize such messages so that they > fit nicely in the popover. For long messages I'd simply wrap the message over multiple lines. I've added an example to the mockups. ... > To keep things simple initially, maybe we can compress logs which have > identical messages and messages that have same beginning word. ... Sounds good to me!
I will start working on implementing the proposed design.
Created attachment 353622 [details] [review] Define GlRowEntry object and it's API The GlRowEntry object includes flags/variables to store information about the compression of journal entries and pass it on from the model to view. There are three types of row entries: uncompressed, compressed, header. If a row entry is a header, then it also stores the compressed journal entries which are represented by the header.
Created attachment 353623 [details] [review] Pass GlRowEntry objects from model to view The model array is populated with GlRowEntry objects. The view classes now interact with GlJournalEntry through GlRowEntry objects.
Created attachment 353624 [details] [review] Compress similar events in GlEventViewList The compression information is stored in GlRowEntry objects during population of the model array.The similarity criteria for grouping adjacent messages is: * Messages whose first word is same * Messages sent by the same process The compression information is then parsed in the view classes and accordingly the events to be compressed are hidden in GlEventViewList. The visibility of compressed events can be toggled by activating the header row representing those compressed events. When compressed events are expanded using the header, sorting order specified by 'sort-order' GSettings key is maintained in GlEventViewList. Headers are removed from the compressed rows to improve their visibility when they are shown by activating the header.
Created attachment 353625 [details] [review] Display detailed event information in a GtkPopover A GtkPopover is shown when a row which is not a header is activated. For handling very long messages, the message text is wrapped to fit the size of the GtkPopover. Further, such long messages are shown in a GtkScrolledWindow.
Review of attachment 353622 [details] [review]: Looks good to me.
Review of attachment 353623 [details] [review]: Looks good to me.
Review of attachment 353624 [details] [review]: In your patch, the colour of header rows stays white. You should add another css class for that. ::: src/gl-journal-model.c @@ +152,3 @@ + model->compressed_entries_counter = 0; + + Please keep the code in else{} indented. @@ +1199,3 @@ + prev_entry_sender = gl_journal_entry_get_command_line (prev_entry); + + const gchar *prev_entry_sender; current_entry_word should be freed. @@ +1205,3 @@ + first_whitespace_index - current_entry_message); + + current_entry = gl_row_entry_get_journal_entry (current_row_entry); prev_entry_word should be freed. @@ +1231,3 @@ + +static void + first_whitespace_index - current_entry_message); Can you explain what this function is used for?
Review of attachment 353625 [details] [review]: @David Any comment on how much information we should keep in the popover compared with what was in the details page? ::: src/gl-eventview.c @@ +254,1 @@ g_object_unref (settings); Some other codes about GL_TYPE_EVENT_VIEW_MODE should be removed as well, as they are no longer useful. ::: src/gl-eventviewdetail.c @@ +45,1 @@ GtkWidget *audit_label; We should keep these information still available in the popover. For example, the time. In list view, seconds are not shown, so we need the detailed time in this popover. @@ +47,3 @@ + GtkWidget *comm_field_label; + GtkWidget *audit_field_label; + GtkWidget *priority_field_label; These information should be kept in the popover.
(In reply to Jonathan Kang from comment #19) > Review of attachment 353625 [details] [review] [review]: > > @David > > Any comment on how much information we should keep in the popover compared > with what was in the details page? > > ::: src/gl-eventview.c > @@ +254,1 @@ > g_object_unref (settings); > > Some other codes about GL_TYPE_EVENT_VIEW_MODE should be removed as well, as > they are no longer useful. > > ::: src/gl-eventviewdetail.c > @@ +45,1 @@ > GtkWidget *audit_label; > > We should keep these information still available in the popover. For > example, the time. In list view, seconds > are not shown, so we need the detailed time in this popover. > > @@ +47,3 @@ > + GtkWidget *comm_field_label; > + GtkWidget *audit_field_label; > + GtkWidget *priority_field_label; > > These information should be kept in the popover. Regarding removal of GlEventView type, I will add a separate patch as it will involve removing the whole GlEventView type and changes to GlWindow type, which will interact with GlEventViewList instead of GlEventView type from now on.
Created attachment 354264 [details] [review] Compress similar events in GlEventViewList Added comments and fixed memory leaks. Moreover, added separate css-class for the compressed header row.
Created attachment 354265 [details] [review] Display detailed event information in a GtkPopover Added all the previously shown detail fields to the GtkPopover.
Review of attachment 354264 [details] [review]: Looks good to me.
Review of attachment 354265 [details] [review]: We'd better break some long messages(for example "Subject" and "Support" field) in the popover into multiple lines, so that scrolling left to right is not needed.
Created attachment 354317 [details] [review] Display detailed event information in a GtkPopover * Added text wrapping property to "Subject" and "Support" label. * Increased the width of GtkPopover to prevent horizontal scrolling.
Created attachment 354318 [details] [review] Remove GlEventView type The GlEventViewDetail type now derives from GtkPopover instead of GtkBin.Hence, The mode setting operation provided by GlEventView type (which is derived from GtkStack) is no longer needed as a GtkStack is not required to switch between GlEventViewList and GlEventViewDetail.
Review of attachment 354317 [details] [review]: ::: src/gl-eventviewdetail.c @@ +60,3 @@ GtkWidget *documentation_field_label; GtkWidget *documentation_label; + GtkWidget *detailed_message_field_label; I'd prefer not adding this label. Leaving as what it was should be fine. The rest looks good.
Review of attachment 354318 [details] [review]: Looks good to me.
Created attachment 354631 [details] [review] Display detailed event information in a GtkPopover * Removed detailed_message_field_label
Review of attachment 354631 [details] [review]: Looks good to me.
Comment on attachment 353622 [details] [review] Define GlRowEntry object and it's API Pushed to master as commit 08a5624b1eb28d8419637a1b655dfe192b71473e.
Comment on attachment 353623 [details] [review] Pass GlRowEntry objects from model to view Pushed to master as commit 0a7fd4c44a7b1cae314aaec0d4048c16d636fe91.
Comment on attachment 354264 [details] [review] Compress similar events in GlEventViewList Pushed to master as commit 3bb233264c9de1a471db86344656ffa77ca9a1e4.
Comment on attachment 354631 [details] [review] Display detailed event information in a GtkPopover Pushed to master as commit 2391b02c0a4cbde9ac1cf313c6ce7741c96be781.
Comment on attachment 354318 [details] [review] Remove GlEventView type Pushed to master as commit ba92386649efbfff25d1dff12b46ef9bf9db4db5.
Thanks everyone who is involved in this bug. Close it as RESOLVED FIXED.
I've tried the new feature and it's a great improvement! Thanks for working on this. There are some things that could be improved: * I find that the tooltips can get in the way, particularly for large log messages. Now that we have the popovers the tooltips feel redundant and I would recommend removing them. * At the moment it is a bit difficult to tell where a series of expanded messages starts and ends. Making the background behind expanded messages darker would help with this. * Expanded message rows don't have a hover effect, like other message rows do. This makes it look like you can't click on them. * The popovers would benefit from some small layout changes: - The grid inside each one needs more spacing around it - try adding a margin of 12px. - There's too much spacing between each row in the popover * The popover uses a mix of monospace and regular text. This gives it a slightly messy appearance. My suggestion would be to use regular text everywhere. * Would it be possible to make the popover size adjust to the size of the content? When messages are very large, it would be nice not to have to scroll too much.
(In reply to Allan Day from comment #37) > I've tried the new feature and it's a great improvement! Thanks for working > on this. > > There are some things that could be improved: > > * I find that the tooltips can get in the way, particularly for large log > messages. Now that we have the popovers the tooltips feel redundant and I > would recommend removing them. > * At the moment it is a bit difficult to tell where a series of expanded > messages starts and ends. Making the background behind expanded messages > darker would help with this. > * Expanded message rows don't have a hover effect, like other message rows > do. This makes it look like you can't click on them. > * The popovers would benefit from some small layout changes: > - The grid inside each one needs more spacing around it - try adding a > margin of 12px. > - There's too much spacing between each row in the popover > * The popover uses a mix of monospace and regular text. This gives it a > slightly messy appearance. My suggestion would be to use regular text > everywhere. > * Would it be possible to make the popover size adjust to the size of the > content? When messages are very large, it would be nice not to have to > scroll too much. Thanks for the feedback. I will be making the changes in a follow-up patch.
(In reply to Allan Day from comment #37) > I've tried the new feature and it's a great improvement! Thanks for working > on this. > > There are some things that could be improved: > > * I find that the tooltips can get in the way, particularly for large log > messages. Now that we have the popovers the tooltips feel redundant and I > would recommend removing them. Sounds good. > * Would it be possible to make the popover size adjust to the size of the > content? When messages are very large, it would be nice not to have to > scroll too much. Seems not. Because some popovers may be a bit high vertically so that it goes beyond the whold window, some information in the popover won't be shown in this case. See the attached image in the next comment for information(There should be two extra label on the top: "Sender systemd"). So I think adding a scrolled window is more reasonable.
Created attachment 354720 [details] Screenshot showing that some information are missing
> Seems not. Because some popovers may be a bit high vertically so that it goes > beyond the whold window, some information in the popover won't be shown in this > case. See the attached image in the next comment for information(There should > be two extra label on the top: "Sender systemd"). That's true. This is the reason why I had to set a fixed size for the detailed popover so that all the details are shown properly and they do not go beyond the window boundaries.
Created attachment 355881 [details] [review] Improvements to event compression UI The UI improvements as suggested by Allan are implemented in this commit: * Make the background behind expanded messages darker. * Add hover effect to the expanded message rows. * Increase grid spacing and decrease row spacing in the detailed popover. * Use regular text everywhere in the detailed popover.
Review of attachment 355881 [details] [review]: ::: data/gl-eventviewdetail.ui @@ +15,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="column-spacing">15</property> try adding a margin of 12px as Allen suggested. @@ +150,3 @@ </packing> </child> + <child>mmm I suppose that "mmm" is here unexpectedly. ::: data/gl-style.css @@ +48,2 @@ .compressed-row-header { + background-color: #575757; It's a bit dark for these styles. I'm not quite into the colour stuff, but what do you think of the following colour scheme. .compressed-row { background-color: #f0f0f0; } .compressed-row:hover { background-color: #e0e0e0; } .compressed-row-header { background-color: #e0e0e0; } .compressed-row-header:hover { background-color: #d0d0d0; }
We spoke with Pranav on IRC about the color scheme and I promised to provide some mockups. Fortunately I already found wireframes that Allan did that expose pretty much everything discussed. https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/logs/logs-message-compression.png I've made a few tweaks that are worth noting -- the expanded log entries arent separated with lines. Only the header line and the whole group is surrounded by a border line. The colors are: #f5f5f5 expanded parent line header background #fafafa expanded group background #d7d7d7 header and group border color @selected_bg_color for the highlighted row background
Created attachment 356116 [details] [review] Improvements to event compression UI The UI improvements as suggested by Allan and Jakub are implemented in this commit: * Use color theme suggested by Jakub for expanded rows. * Add hover effect to the expanded message rows. * Increase grid spacing and decrease label padding in popover grid. * Use regular text everywhere in the detailed popover. * Add margin for grid within the detailed popover. * Highlight the row pointed by detailed popover.
(In reply to Jakub Steiner from comment #44) > We spoke with Pranav on IRC about the color scheme and I promised to provide > some mockups. Fortunately I already found wireframes that Allan did that > expose pretty much everything discussed. > > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > logs/logs-message-compression.png > > I've made a few tweaks that are worth noting -- the expanded log entries > arent separated with lines. Only the header line and the whole group is > surrounded by a border line. The colors are: > > #f5f5f5 expanded parent line header background > #fafafa expanded group background > #d7d7d7 header and group border color > @selected_bg_color for the highlighted row background Thanks for the awesome work on the tweak to the previous design! It looks pretty good.
Review of attachment 356116 [details] [review]: I made some small changes and pushed to master as commit 0552d30daf00398b8660645e5d383facb8b6d068.