GNOME Bugzilla – Bug 767995
Move searching functionality from view to model
Last modified: 2017-05-30 14:09:49 UTC
Created attachment 330290 [details] [review] Patch for moving the searching functionality from view to model Currently, the searching functionality is implemented in the front end side , more specifically in the GlEventViewList module. But this has it's downsides such that we are able to search from logs shown only in the current view and not from all the possible logs within the specified boot time range. The search functionality is implemented using the GtkListBox filter functions which search only from the shown logs. So, we move the search functionality to the GlJournalModel module which is the actual back end using an intermediate Query API which passes the match fields from GlEventViewList to GlJournalModel module and hence, enable searching from all the possible logs within the selected boot time range and at the same time reducing complexity from GlEventViewList module.
Created attachment 330295 [details] [review] Defined structs for GlQuery and GlQueryItem
Created attachment 330296 [details] [review] Removed property 'PROP_MATCHES' from journal model
Created attachment 330297 [details] [review] Moved querying for category matches to journal model using query object
Created attachment 330298 [details] [review] Moved search matches to journal model using query API
Review of attachment 330295 [details] [review]: The commit message body is too long. It should be limited to 72 characters per line. ::: src/gl-journal-model.c @@ +280,3 @@ + query = g_slice_new (GlQuery); + + query->queryitems = g_ptr_array_new_with_free_func ( (GDestroyNotify) gl_query_item_free); There is an extra space before the (GDestroyNotify). ::: src/gl-journal-model.h @@ +25,3 @@ +typedef struct GlQuery +{ + GPtrArray *queryitems; /* array of GlQueryItem objects passed through eventviewlist */ This does not say much more than the comment above it. Take out the "passed through eventviewlist". Additionally, a GlQueryItem is not an object - it is just a struct.
Review of attachment 330296 [details] [review]: Seems fine.
Review of attachment 330297 [details] [review]: The commit message body needs reflowing to have a maximum line length of 72 characters. ::: src/gl-eventviewlist.c @@ +742,3 @@ +} + +/* Create Query Object according to GUI elements and set it on Journal Model */ Check capitalisation. Also, it is more correct to say that that the query object depends on the category, not some unspecified "GUI elements". @@ +761,3 @@ + gl_query_add_match (query, "_BOOT_ID", boot_id); + + /* Add Exact Matches according to selected category */ No need to capitalise Exact Matches. ::: src/gl-journal-model.c @@ +284,3 @@ + GPtrArray *matches; + + matches = g_ptr_array_new_with_free_func ( (GDestroyNotify) g_free); There is an extra space. @@ +322,3 @@ + GlQuery *query) +{ + g_return_if_fail (GL_IS_JOURNAL_MODEL (model)); Use GL_JOURNAL_MODEL instead of GL_IS_JOURNAL_MODEL. ::: src/gl-journal-model.h @@ +36,3 @@ +void gl_journal_model_take_query (GlJournalModel *model, + GlQuery *query); Missing a space here. @@ +40,3 @@ +void gl_query_add_match (GlQuery *query, + gchar *field_name, + gchar *field_value); It is uncommon to take ownership of a string that is passed in to a function. If this is intended, it should be documented in a comment. ::: src/gl-journal.c @@ +567,3 @@ void gl_journal_set_matches (GlJournal *journal, + GPtrArray *matches) The spacing should be adjusted for the new (shorter) arguments. ::: src/gl-journal.h @@ +72,3 @@ GType gl_journal_result_get_type (void); GType gl_journal_get_type (void); +void gl_journal_set_matches (GlJournal *journal, GPtrArray *matches); You could avoid changing the argument in this patch, as you can instead use "pdata" inside the GPtrArray struct as before, although you would need to check that the array was correctly terminated.
Review of attachment 330298 [details] [review]: Commit message body formatting. ::: src/gl-journal-model.c @@ +60,3 @@ +{ + LOGICAL_OR = 2, + LOGICAL_AND = 3 Is there any reason to give these enum constants a value? @@ +557,3 @@ + + else if (utf8_strcasestr ("_kernel_device", search_match->field_name)) return kernel_device; + Weird extra line breaks here and above - they should be removed. @@ +567,3 @@ +gl_query_item_get_match (GlQueryItem *search_match, + const gchar *field_value, + gboolean case_sensetive) Typo: sensitive. @@ +571,3 @@ + if (field_value) + { + if (case_sensetive) Typo. @@ +636,3 @@ + + /* No logical AND or OR used in search text */ + if(token_array->len == 1) Need an extra space after the "if". @@ +641,3 @@ + } + + /* If multiple Tokens are present : execute the token mode */ Capitalisation. @@ +769,3 @@ + else + { + search_text_copy = g_strdup (search_match->field_value); Is it necessary to duplicate the string for the tokenizing?
Created attachment 331513 [details] [review] Defined structs for GlQuery and GlQueryItem These structs will be used for passing data from eventviewlist (view) to journal model. Also, the functions for allocating and freeing them are defined.
Created attachment 331540 [details] [review] Define structs for query API These structs will be used for passing data from eventviewlist (view) to journal model. Also, the functions for allocating and freeing them are defined.
Created attachment 331541 [details] [review] Move querying for category matches to journal model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331542 [details] [review] Pass GPtrArray to gl_journal_set_matches() The function argument is changed from const gchar * const * to GPtrArray.
Created attachment 331543 [details] [review] Move search matches to journal model using query API The querying for user entered search text is now done by parsing the query object in journal model and it is populated accordingly. The query object is populated in the view. Earlier, the searching work was done by listbox_search_filter_func()but now it is transferred to the query API. A new enum 'GlSearchQueryType' was introduced to differentiate between exact and substring matches.The search related functions from eventviewlist are also removed.
Created attachment 331544 [details] [review] Move querying for category matches to journal model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331545 [details] [review] Pass GPtrArray to gl_journal_set_matches() The function argument is changed from const gchar * const * to GPtrArray.
Created attachment 331546 [details] [review] Move search matches to journal model using query API The querying for user entered search text is now done by parsing the query struct in journal model and it is populated accordingly. The query struct is populated in the view. Earlier, the searching work was done by listbox_search_filter_func() but now it is transferred to the query API. A new enum 'GlSearchQueryType' was introduced to differentiate between exact and substring matches.The search related functions from eventviewlist are also removed.
Created attachment 331558 [details] [review] Define structs for query API These structs will be used for passing data from eventviewlist (view) to journal model. Also, the functions for allocating and freeing them are defined.
Created attachment 331559 [details] [review] Move search matches to journal model using query API The querying for user entered search text is now done by parsing the query struct in journal model and it is populated accordingly. The query struct is populated in the view.Earlier, the searching work was done by listbox_search_filter_func() but now it is transferred to the query API. A new enum 'GlSearchQueryType' was introduced to differentiate between exact and substring matches.The search related functions from eventviewlist are also removed.
Review of attachment 331544 [details] [review]: Looks like the first line of the commit message is longer than 50 characters. Please keep to that limit.
Review of attachment 331545 [details] [review]: Looks good.
Review of attachment 331558 [details] [review]: Looks good.
Review of attachment 331559 [details] [review]: ::: src/gl-journal-model.c @@ +58,3 @@ +/* We define these two enum values as 2 and 3 to avoid the + * conflict with TRUE and FALSE */ Why do you need to avoid a conflict with TRUE and FALSE? Also, given that FALSE is defined as 0 and TRUS is defined as !FALSE, this does not seem to apply.
Review of attachment 331559 [details] [review]: ::: src/gl-journal-model.c @@ +58,3 @@ +/* We define these two enum values as 2 and 3 to avoid the + * conflict with TRUE and FALSE */ It seems like this code already existed, so the patch seems fine.
Created attachment 331564 [details] [review] Move category matches to model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331565 [details] [review] Move search matches to model using query API The querying for user entered search text is now done by parsing the query struct in journal model and it is populated accordingly. The query struct is populated in the view.Earlier,the searching work was done by listbox_search_filter_func() but now it is transferred to the query API. A new enum 'GlSearchQueryType' was introduced to differentiate between exact and substring matches.The search related functions from eventviewlist are also removed.
Review of attachment 331564 [details] [review]: ::: src/gl-journal.c @@ +566,3 @@ */ void +gl_journal_set_matches (GlJournal *journal, This should be changed when the argument changes, not in this commit.
Review of attachment 331565 [details] [review]: Seems fine.
Created attachment 331580 [details] [review] Move category matches to model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331581 [details] [review] Move category matches to model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331582 [details] [review] Pass GPtrArray to gl_journal_set_matches() The function argument is changed from const gchar * const * to GPtrArray.
Review of attachment 331581 [details] [review]: ::: src/gl-journal-model.c @@ +288,3 @@ + g_ptr_array_foreach (query->queryitems, (GFunc) get_exact_match_string, matches); + + /* Add null terminator */ "NULL" not "null", although explaining why it is necessary to terminate with a NULL is more useful than a literal explanation of the following code. @@ +303,3 @@ + category_matches = gl_query_get_exact_matches (model->query); + + gl_journal_set_matches (model->journal, (const gchar * const*) category_matches->pdata); "const *" not "const*"
Review of attachment 331582 [details] [review]: Looks good.
Created attachment 331586 [details] [review] Move category matches to model using query API Query struct is populated with the corresponding category matches.It is parsed in the journal model to finally apply the matches with gl_journal_set_matches().After applying the matches, journal is repopulated accordingly.
Created attachment 331587 [details] [review] Pass GPtrArray to gl_journal_set_matches() The function argument is changed from const gchar * const * to GPtrArray.
Review of attachment 331586 [details] [review]: Looks good.
Review of attachment 331587 [details] [review]: Good!
Comment on attachment 330296 [details] [review] Removed property 'PROP_MATCHES' from journal model Pushed to master as commit 17d2adb37b91aba2045e8ba983295cabb63c64d4.
Review of attachment 331558 [details] [review]: Pushed to master as commit 06546ec2708283388cc7f8ae5e329fa69e931c77.
Review of attachment 331586 [details] [review]: Pushed to master as commit b2505ca31a57e77afc6822ca6134399f9705d15d.
Review of attachment 331587 [details] [review]: Pushed to master as commit ce9263e17eba6db4da80b61efe58bc986fd27978.
Review of attachment 331565 [details] [review]: Pushed to master as commit b86a50a312b08dadbd58e04db351c38c65927b46.
The remaining work to finish this off is to stop using filtering and sorting on the view list box, and instead do that in the model.
Created attachment 331790 [details] [review] Remove sort and filter functions from listbox The sort and filter functions were removed as the querying is now transferred to model.
Review of attachment 331790 [details] [review]: Clearly this is wrong, as the model does not automatically react to changes in the sort-order GSettings key. Additionally, the patch introduces a compiler warning thanks to an unused variable.
Created attachment 346093 [details] [review] Move sorting from view to model The logs shown in the eventviewlist are now sorted from the model. The logs can be sorted in ascending or descending order according to the timestamp. The sorting order can be controlled by setting the 'sort-order' GSettings schema.
Review of attachment 346093 [details] [review]: This does not fill up the internal model (the array) in a uniform order, as I suggested. It is not necessary to add new API such as gl_journal_next(), and you should instead always populate the model iterating backwards, and adjust the g_list_model_items_changed() calls accordingly. ::: src/gl-journal-model.h @@ +32,3 @@ + GL_QUERY_JOURNAL_MODEL_ORDER_DESCENDING, + GL_QUERY_JOURNAL_MODEL_ORDER_ASCENDING +} GlQueryJournalModelOrder; Just use GlSortOrder instead.
(In reply to David King from comment #47) > Review of attachment 346093 [details] [review] [review]: > > This does not fill up the internal model (the array) in a uniform order, as > I suggested. It is not necessary to add new API such as gl_journal_next(), > and you should instead always populate the model iterating backwards, and > adjust the g_list_model_items_changed() calls accordingly. > > ::: src/gl-journal-model.h > @@ +32,3 @@ > + GL_QUERY_JOURNAL_MODEL_ORDER_DESCENDING, > + GL_QUERY_JOURNAL_MODEL_ORDER_ASCENDING > +} GlQueryJournalModelOrder; > > Just use GlSortOrder instead. I am not able to get it from the documentation for g_list_model_items_changed() on how it is possible to display the items in an ascending order for a array bound to the model which is initially populated in descending order. Maybe we can use "GListStore" here which provides inbuilt sorting capabilities through it's API.
I am not able to think of another approach here which does not involve modifiying the GPtrArray bound to the GlJournalModel. Any suggestions or guidance on this ?
Created attachment 351469 [details] [review] Move sorting from view to model Updated the patch to populate the GPtrArray in uniform descending order and sort the GPtrArray by modifying the *_get_item() implementation.
Review of attachment 351469 [details] [review]: The commit message is incorrect, as 'sort-order' is a GSettings key, not a schema. ::: src/gl-eventviewlist.c @@ +700,2 @@ + /* Set the created query on the journal model */ + gl_journal_model_take_query (priv->journal_model, query); This now looks rather strange: there is no indication of how creating a new query sets the sort order. There should be a comment explaining that the current sort order (from GSettings, which may be different that the current model sort order) is taken when creating a new query. ::: src/gl-journal-model.c @@ +202,3 @@ + { + /* read the array in reverse direction */ + return g_object_ref (g_ptr_array_index (model->entries, (model->entries->len - 1) - position)); Does this work if there are no entries in the array?
Created attachment 352115 [details] [review] Move sorting from view to model Made the changes as suggested in the review.
Review of attachment 352115 [details] [review]: On testing this, the model sorting seems to work fine. However, adding new log entries to the view does not work correctly, as those are only fetched when scrolling to the bottom of the view (which when the oldest entry is on top, is broken). This is unrelated to the patch, just something that came up during testing, and should be reported and solved in a different bug. One problem with the patch is that several lines are longer than 79 characters - check those and resubmit, please.
I think for model sorting to work correctly, we need to fetch the entries from the journal itself in an ascending order. This is something I had already implemented using gl_journal_next() in a previous patch.
Created attachment 352869 [details] [review] Move sorting from view to model Shortened lines greater than 79 characters.
Review of attachment 352869 [details] [review]: Thanks! Pushed to master as commit 4aecdf242964aa4c3817b565bb25c6708a29dbb5.