GSoC History API

Review Request #877 — Created Aug. 5, 2021 and submitted

rewtguy
pidgin/pidgin
default
PIDGIN-17526, PIDGIN-17532, PIDGIN-17533, PIDGIN-17534
876
f2d5e64fd957
pidgin

GSoC History API including sqlite history adapter

The History API has been created to drive all message handling in purple3. It will be used to update existing messages for edits, reactions, pinning, read/deliver receipts, etc. The API uses an adapter pattern, to abstract out backends, but provides a SQLite3 backend by default.

It also provides search capabilities using a custom query language that can easily be expanded over time. It will be use by both the end user to search messages and the frontends to implement features like a pinned messages button. A command line utility is also provided for searching outside of the program itself.

Remaining Items

These all will most likely be done by the Pidgin core team after GSoC when we figure out exactly how to solve them.

Need to store database in purple config directory
* Gary has spent some time looking at this and it looks like the purple-history cli will need to become a purple-ui to make this work write as in the future other adapters will be plugins.

Other things to consider:
- For simplicity, the SqliteHistoryAdapter is parsing the query itself, but for consistency having PurpleHistoryAdapter parse the query and pass tokens to the subclass might be something we want to do.

Unit Tests

History Manager
History Adapter

Integration Tests

purplehistorycore created for integration tests.
PurpleSqliteHistoryAdapter functionality tested:
- Creates proper db schema
- Writes logs
- Reads logs
- Queries using query language
- Deletes using query language

Description From Last Updated

I have only read tests and integration so far, not implementation.

QuLogicQuLogic

Comment block looks like it's missing a space for alignment

sorvivalsorvival

whitespace issues.

grimgrim

whitespace issues

grimgrim

whitespace issues

grimgrim

Same thing about comment block missing an alignment space

sorvivalsorvival

indentation

grimgrim

you don't need the #gchar here.

grimgrim

I would say something like "runs @query against @adapter">

grimgrim

Perhaps "A list of message that match @query." reads better.

grimgrim

Missing @query

grimgrim

"Tells @adapter to remove messages that match @query."

grimgrim

"If removing the messages was successful". I wonder if we should make this return an int of how many messages …

grimgrim

These could probably just read %s does not implement the ... function.

grimgrim

There should always be a newline after variable declarations.

grimgrim

wrong indentation

grimgrim

missing active_changed.

grimgrim

This should be something more like "Removes messages from the active #PurpleHistoryAdapter of @manager that match @query."

grimgrim

"removed from the active adapter of @manager."

grimgrim

The #PurpleConversation instance. is plenty.

grimgrim

"Writes @message to the active adapter of @manager."

grimgrim

"%TRUE if @message was successfully written, %FALSE otherwise."

grimgrim

these should be in the same order they are in the Class structure so SIG_ACTIVE_CHANGED should be first.

grimgrim

Not sure how we missed a property for active, but we should probably add it.

grimgrim

This should be moved to line 270 so we don't need to unreference it if we're returning early because it's …

grimgrim

{ should be on a new line.

grimgrim

No need for the cannot perform query, bits in these. Who ever displays the error is likely going to put …

grimgrim

No need for the cannot perform remove, bits in these. Who ever displays the error is likely going to put …

grimgrim

No need for the cannot write message, bits in these. Who ever displays the error is likely going to put …

grimgrim

SQLite is the official capitalization.

grimgrim

don't need the extra blank lines here.

grimgrim

{ should be on a new line as the function parameters were wrapped.

grimgrim

`/ be careful with TRUE as it'll delete everything if theres nothing else in the query. /

grimgrim

we need to g_list_free_full(ins, g_free); each of the 3 lists here.

grimgrim

Might be nice to point out that sqlite prepared statements counts it's parameters starting at 1.

grimgrim

wrong indentation

grimgrim

wrong indentation

grimgrim

{ should be on a new line.

grimgrim

{ should be on a new line.

grimgrim

The check from purple_sqlite_history_adapter_write about checking if priv->db == NULL should be in here too.

grimgrim

we need to finish these.

grimgrim

{ should be on a new line.

grimgrim

The check from purple_sqlite_history_adapter_write about checking if priv->db == NULL should be in here too.

grimgrim

TODO

grimgrim

{ should be on a new line.

grimgrim

return should have an empty line above it unless it's the only statement in a block.

grimgrim

unnecessary newline

grimgrim

{ should be on the previous line as it didn't wrap.

grimgrim

no need for this to be a variable, just put the string in the function call.

grimgrim

need to add purplesqlitehistoryadapter.c as well.

grimgrim

Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

grimgrim

Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

grimgrim

Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

grimgrim

Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

grimgrim

This list shouldn't be sorted.

grimgrim

This should be transfer full and it's returning a list of PurpleMessage's not PurpleHistoryAdapter's.

grimgrim

newly activated

grimgrim

s/from/for/ ?

grimgrim

#PurpleHistoryAdapter

grimgrim

PurpleHistoryAdapter

grimgrim

should be } else {

grimgrim

the { shouldn't be indented

grimgrim

{ should be on a new line when parameters wrap.

grimgrim

{ should be on a new line when parameters wrap.

grimgrim

{ should be on a new line when parameters wrap.

grimgrim

{ should be on a newline when parameters wrap.

grimgrim

use purple_strequal insteqad of strcmp and all four of these.

grimgrim

return's should be separated by a newline unless they are the only statement in a block.

grimgrim

Looks like mixed indentation here.

grimgrim

This should be indented to the level of what it is documenting.

grimgrim

Extra space after (?

QuLogicQuLogic

Clear at end of test.

QuLogicQuLogic

Clear at end of test.

QuLogicQuLogic

Clear at end of test.

QuLogicQuLogic

Clear at end of test.

QuLogicQuLogic

replace the query variable with "query" there's no need for this to be a variable.

grimgrim

Clear at end of test.

QuLogicQuLogic

Clear all these at end of test.

QuLogicQuLogic

too many newlines

grimgrim

you should be checking the return vaolue of purple_history_manager_remove as well.

grimgrim

you should be checking the return vaolue of purple_history_manager_write as well.

grimgrim

Fix inconsistent names, m->manager, e->error, etc.

QuLogicQuLogic

Ditto.

QuLogicQuLogic

This should be query, not test-adapter, I think.

QuLogicQuLogic

Ditto.

QuLogicQuLogic

Aren't these all the defaults anyway?

QuLogicQuLogic

Ditto.

QuLogicQuLogic

This should go up near GLib and such, not after all the prpl-specific checks.

QuLogicQuLogic

Why is this top-level and not in libpurple somewhere??

QuLogicQuLogic

This should make the command itself exit with EXIT_FAILURE.

QuLogicQuLogic

message needs to be unref'd before the link is deleted.

grimgrim

"failed to remove"

QuLogicQuLogic

Should also exit with failure.

QuLogicQuLogic

I think you can add g_option_context_set_help_enabled on the option context.

QuLogicQuLogic

This check is redundant with the for loop condition, since you don't do anything different if argc <= 1. You …

QuLogicQuLogic

THe

grimgrim

should be wrapped.

grimgrim

should be #PurpleHistoryAdapter's

grimgrim

should be #PurpleHistoryAdapter's as #PurpleHistoryAdapters isn't a known symbol to gtk-doc.

grimgrim

Should be #PurpleMessage's

grimgrim

the signals name is registered not adapter-registered so this should be PurpleHistoryManager::registered:

grimgrim

the signals name is unregistered not adapter-unregistered so this should be PurpleHistoryManager::unregistered:

grimgrim

should be wrapped

grimgrim

parameters wrapped so { should be on a newline.

grimgrim

no need to mention what it subclasses PurpleHistoryAdapter

grimgrim

"the instance read and writes to" is redundant and should be removed.

grimgrim

"Creates a new #PurpleSqliteHistoryAdapter."

grimgrim

"(transfer full): The new #PurpleSqliteHistoryAdapter instance"

grimgrim

The parameter documentation shouldn't contain the description of the function. It just needs to include the information about the parameter. …

grimgrim

You need a description here that is separated by a newlines between the parameters and the Returns. Something like *@adapter: …

grimgrim

extra space after *

grimgrim

we should be checking if these are empty strings before adding them to the lists and incrementing the query_items counter. …

grimgrim

need to free the lists here too.

grimgrim

This should be wrapped... something like. g_set_error(error, PURPLE_HISTORY_ADAPTER_DOMAIN, 0, _("Error opening database in PurpleSqliteHistoryAdapter " "for file %s"), priv->filename);

grimgrim

We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows …

grimgrim

We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows …

grimgrim

with the previous comment, having a variable of type PurpleSqliteHistoryAdapter avoid this line going past the boundary.

grimgrim

We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows …

grimgrim

again, use the variable that the previous comment will create to avoid having a line that goes passed the boundary.

grimgrim

We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows …

grimgrim

these all should be line wrapped.

grimgrim

putting sqlite3_errmsg(priv->db)); on a new line will get this line under 80 chars.

grimgrim

You don't need the PURPLE_HISTORY_ADAPTER() here.

grimgrim

extra newline

grimgrim

Since TestPurpleHistoryAdapter->query returns a list with 1 item you need to free it after the check with g_list_free.

grimgrim

uncomment and fix typo.

grimgrim

looks like this doesn't need to be wrapped?

grimgrim

looks like this doesn't need to be wrapped?

grimgrim

should be wrapped when over 80 chars.

grimgrim

should be wrapped after 80 chars

grimgrim

you can drop the PURPLE_MESSAGE()

grimgrim

You can drop the PURPLE_CONVERSATION()

grimgrim

This should wrap at 80 chars

grimgrim

previous and current are PurpleHistoryAdapter's not PurpleHistoryManager's.

grimgrim

There should be a space between sqlite3_stmt and the *

grimgrim

should be gint

grimgrim

there should be a new line before return when it's not the only statement in a block.

grimgrim

these three g_list_free_full's can be deleted now as the lists are cleaned up in the loops above and we've transfered …

grimgrim

there should not be a space between * and prepared_statement.

grimgrim

Keep math last.

QuLogicQuLogic

Missing the PURPLE_IS_HISTORY_ADAPTER check.

QuLogicQuLogic

Missing the PURPlE_IS_HISTORY_ADAPTER check.

QuLogicQuLogic

Returns:

QuLogicQuLogic

Returns:

QuLogicQuLogic

Extra space.

QuLogicQuLogic

The old one is deactivated at this point; should it be re-activated?

QuLogicQuLogic

Should be g_set_error_literal

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

Should this be checking conversation or message, or is that dependent on specific adapters?

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

SQLite belongs in the title?

QuLogicQuLogic

SQLite

QuLogicQuLogic

is a class

QuLogicQuLogic

defines inherits?

QuLogicQuLogic

Does something actually derive from this? It should be final if not.

QuLogicQuLogic

error is leaked.

QuLogicQuLogic

error is already set by g_resource_lookup_data, and error_msg is empty?

QuLogicQuLogic

purple_strequal returns TRUE when they're equal; this should not be comparing == 0.

QuLogicQuLogic

This is just split[i][3].

QuLogicQuLogic

split[i][5]

QuLogicQuLogic

split[i][0]

QuLogicQuLogic

Since these are all ORd together, it doesn't appear that there's any benefit to putting this in any 'correct' order, …

QuLogicQuLogic

Should be g_error_set_literal

QuLogicQuLogic

If query_items is 0, then none of these would have anything in them.

QuLogicQuLogic

It would be shorter to write this part of the query as converation_id IN (?, ?, ?, ...).

QuLogicQuLogic

The whitespace probably doesn't matter, but did you intend to put newlines after each type of qualifier?

QuLogicQuLogic

Same here with author IN (?)

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

priv->filename == NULL, so no point in including it in the error message like this, instead of explicitly calling that …

QuLogicQuLogic

If running migrations fails, then don't you need to close the db handle?

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

Instead of SELECT *, you probably should have selected these specific columns. Then you don't need to pick random indices …

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

Should you not finalize the prepared statement on error?

QuLogicQuLogic

This is const.

QuLogicQuLogic

Should be g_set_error_literal.

QuLogicQuLogic

Why is this not using the existing message_id?

QuLogicQuLogic

Should you not finalize the prepared statement on error?

QuLogicQuLogic

This needs to clear the db handle (and maybe warn) if it wasn't correctly unregistered.

QuLogicQuLogic

You should pass an error to all these tests and g_assert_no_error.

QuLogicQuLogic

Query is transfer container, not transfer full. Also, the test adapter returns GINT_TO_POINTER(1), so should definitely not be freed.

QuLogicQuLogic

Extra line break.

QuLogicQuLogic

Should be TRUE now.

QuLogicQuLogic

This is unused?

QuLogicQuLogic

Should be TRUE now.

QuLogicQuLogic

You aren't checking the return value to exit with the failure return code.

QuLogicQuLogic

Clear error.

QuLogicQuLogic

This indent seems off.

QuLogicQuLogic

It seems that adding these annotations for the error is unnecessary since the .gir doesn't not record it directly as …

QuLogicQuLogic

Is this opening in the current directory? It should go to the user config dir.

QuLogicQuLogic

Can drop the cast.

QuLogicQuLogic

Should verify that old is not NULL/non-adapter.

QuLogicQuLogic

Why are there two step calls?

QuLogicQuLogic

Here are the other ones.

QuLogicQuLogic
grim
grim
  1. 
      
  2. libpurple/purplehistorymanager.c (Diff revision 1)
     
     

    whitespace issues.

  3. libpurple/purplehistorymanager.c (Diff revision 1)
     
     

    whitespace issues

  4. libpurple/purplehistorymanager.c (Diff revision 1)
     
     

    whitespace issues

  5. libpurple/purplesqlitehistoryadapter.c (Diff revision 1)
     
     

    indentation

  6. 
      
sorvival
  1. 
      
  2. libpurple/purplehistorymanager.h (Diff revision 1)
     
     

    Comment block looks like it's missing a space for alignment

  3. libpurple/purplesqlitehistoryadapter.h (Diff revision 1)
     
     

    Same thing about comment block missing an alignment space

  4. 
      
rewtguy
grim
  1. 
      
  2. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    you don't need the #gchar here.

  3. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    I would say something like "runs @query against @adapter">

  4. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    Perhaps "A list of message that match @query." reads better.

  5. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    Missing @query

  6. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    "Tells @adapter to remove messages that match @query."

  7. libpurple/purplehistoryadapter.h (Diff revision 2)
     
     

    "If removing the messages was successful".

    I wonder if we should make this return an int of how many messages were removed.

  8. libpurple/purplehistoryadapter.c (Diff revision 2)
     
     

    These could probably just read %s does not implement the ... function.

  9. libpurple/purplehistoryadapter.c (Diff revision 2)
     
     

    There should always be a newline after variable declarations.

  10. libpurple/purplehistoryadapter.c (Diff revision 2)
     
     

    wrong indentation

  11. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    missing active_changed.

  12. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    This should be something more like

    "Removes messages from the active #PurpleHistoryAdapter of @manager that match @query."

  13. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    "removed from the active adapter of @manager."

  14. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    The #PurpleConversation instance. is plenty.

  15. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    "Writes @message to the active adapter of @manager."

  16. libpurple/purplehistorymanager.h (Diff revision 2)
     
     

    "%TRUE if @message was successfully written, %FALSE otherwise."

  17. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    these should be in the same order they are in the Class structure so SIG_ACTIVE_CHANGED should be first.

  18. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    Not sure how we missed a property for active, but we should probably add it.

  19. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    This should be moved to line 270 so we don't need to unreference it if we're returning early because it's the active adapter.

  20. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    { should be on a new line.

  21. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    No need for the cannot perform query, bits in these. Who ever displays the error is likely going to put that message there.

  22. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    No need for the cannot perform remove, bits in these. Who ever displays the error is likely going to put that message there.

  23. libpurple/purplehistorymanager.c (Diff revision 2)
     
     

    No need for the cannot write message, bits in these. Who ever displays the error is likely going to put that message there.

  24. libpurple/purplesqlitehistoryadapter.h (Diff revision 2)
     
     

    SQLite is the official capitalization.

  25. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    don't need the extra blank lines here.

  26. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    { should be on a new line as the function parameters were wrapped.

  27. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    `/ be careful with TRUE as it'll delete everything if theres nothing else in the query. /

  28. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    we need to g_list_free_full(ins, g_free); each of the 3 lists here.

  29. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    Might be nice to point out that sqlite prepared statements counts it's parameters starting at 1.

  30. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    wrong indentation

  31. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    wrong indentation

  32. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    { should be on a new line.

  33. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    { should be on a new line.

  34. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    The check from purple_sqlite_history_adapter_write about checking if priv->db == NULL should be in here too.

  35. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    we need to finish these.

  36. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    { should be on a new line.

  37. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    The check from purple_sqlite_history_adapter_write about checking if priv->db == NULL should be in here too.

  38. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    { should be on a new line.

  39. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    return should have an empty line above it unless it's the only statement in a block.

  40. libpurple/purplesqlitehistoryadapter.c (Diff revision 2)
     
     

    unnecessary newline

  41. libpurple/tests/test_history_adapter.c (Diff revision 2)
     
     

    { should be on the previous line as it didn't wrap.

  42. libpurple/tests/test_history_adapter.c (Diff revision 2)
     
     

    no need for this to be a variable, just put the string in the function call.

  43. po/POTFILES.in (Diff revision 2)
     
     

    need to add purplesqlitehistoryadapter.c as well.

  44. 
      
rewtguy
grim
  1. 
      
  2. libpurple/purplehistoryadapter.c (Diff revision 3)
     
     

    Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

  3. libpurple/purplehistoryadapter.c (Diff revision 3)
     
     

    Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

  4. libpurple/purplehistoryadapter.c (Diff revision 3)
     
     

    Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

  5. libpurple/purplehistoryadapter.c (Diff revision 3)
     
     

    Should just use the same %s does not implement the ... function like purple_history_adapter_query does.

  6. libpurple/purplehistorymanager.h (Diff revision 3)
     
     

    This list shouldn't be sorted.

  7. libpurple/purplehistorymanager.h (Diff revision 3)
     
     

    This should be transfer full and it's returning a list of PurpleMessage's not PurpleHistoryAdapter's.

  8. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    newly activated

  9. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    s/from/for/ ?

  10. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    #PurpleHistoryAdapter

  11. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    PurpleHistoryAdapter

  12. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    should be } else {

  13. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    the { shouldn't be indented

  14. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    { should be on a new line when parameters wrap.

  15. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    { should be on a new line when parameters wrap.

  16. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    { should be on a new line when parameters wrap.

  17. libpurple/purplehistorymanager.c (Diff revision 3)
     
     

    { should be on a newline when parameters wrap.

  18. libpurple/purplesqlitehistoryadapter.c (Diff revision 3)
     
     

    use purple_strequal insteqad of strcmp and all four of these.

  19. libpurple/purplesqlitehistoryadapter.c (Diff revision 3)
     
     

    return's should be separated by a newline unless they are the only statement in a block.

  20. libpurple/purplesqlitehistoryadapter.c (Diff revision 3)
     
     

    Looks like mixed indentation here.

  21. libpurple/purplesqlitehistoryadapter.c (Diff revision 3)
     
     

    This should be indented to the level of what it is documenting.

  22. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    replace the query variable with "query" there's no need for this to be a variable.

  23. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    too many newlines

  24. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     

    you should be checking the return vaolue of purple_history_manager_remove as well.

  25. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     

    you should be checking the return vaolue of purple_history_manager_write as well.

  26. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    message needs to be unref'd before the link is deleted.

  27. 
      
QuLogic
  1. 
      
  2. I have only read tests and integration so far, not implementation.

  3. Extra space after (?

  4. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    Clear at end of test.

  5. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    Clear at end of test.

  6. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    Clear at end of test.

  7. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    Clear at end of test.

  8. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     

    Clear at end of test.

  9. libpurple/tests/test_history_adapter.c (Diff revision 3)
     
     
     
     
     
     

    Clear all these at end of test.

  10. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     
     
     
     
     
     

    Fix inconsistent names, m->manager, e->error, etc.

  11. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     
     
     
     
     
  12. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     

    This should be query, not test-adapter, I think.

  13. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     
     
     
     
     
     
     
  14. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     
     
     
     
     

    Aren't these all the defaults anyway?

  15. libpurple/tests/test_history_manager.c (Diff revision 3)
     
     
     
     
     
     
     
     
  16. meson.build (Diff revision 3)
     
     

    This should go up near GLib and such, not after all the prpl-specific checks.

  17. meson.build (Diff revision 3)
     
     

    Why is this top-level and not in libpurple somewhere??

    1. because I still plan on splitting the libpurple and finch out of this repo so we put it at the toplevel where it would be after that split.

  18. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    This should make the command itself exit with EXIT_FAILURE.

  19. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    "failed to remove"

  20. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    Should also exit with failure.

  21. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    I think you can add g_option_context_set_help_enabled on the option context.

  22. purple-history/purplehistorycore.c (Diff revision 3)
     
     

    This check is redundant with the for loop condition, since you don't do anything different if argc <= 1.

    You can declare i in the for loop now to keep its scope down.

  23. 
      
rewtguy
grim
  1. 
      
  2. libpurple/purplehistorymanager.h (Diff revision 4)
     
     

    should be wrapped.

  3. libpurple/purplehistorymanager.h (Diff revision 4)
     
     

    should be #PurpleHistoryAdapter's

  4. libpurple/purplehistorymanager.h (Diff revision 4)
     
     

    should be #PurpleHistoryAdapter's as #PurpleHistoryAdapters isn't a known symbol to gtk-doc.

  5. libpurple/purplehistorymanager.h (Diff revision 4)
     
     

    Should be #PurpleMessage's

  6. libpurple/purplehistorymanager.c (Diff revision 4)
     
     

    the signals name is registered not adapter-registered so this should be PurpleHistoryManager::registered:

  7. libpurple/purplehistorymanager.c (Diff revision 4)
     
     

    the signals name is unregistered not adapter-unregistered so this should be PurpleHistoryManager::unregistered:

  8. libpurple/purplehistorymanager.c (Diff revision 4)
     
     

    should be wrapped

  9. libpurple/purplehistorymanager.c (Diff revision 4)
     
     

    parameters wrapped so { should be on a newline.

  10. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    no need to mention what it subclasses PurpleHistoryAdapter

  11. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    "the instance read and writes to" is redundant and should be removed.

  12. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    "Creates a new #PurpleSqliteHistoryAdapter."

  13. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    "(transfer full): The new #PurpleSqliteHistoryAdapter instance"

  14. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    The parameter documentation shouldn't contain the description of the function. It just needs to include the information about the parameter. In this case The #PurpleSqliteHistoryAdapter instance.

  15. libpurple/purplesqlitehistoryadapter.h (Diff revision 4)
     
     

    You need a description here that is separated by a newlines between the parameters and the Returns.

    Something like

     *@adapter: The #PurpleSqliteHistoryAdapter instance.
     *
     * Gets the filename of the sqlite database.
     *
     * Returns: The file name of the sqlite data.
    
  16. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    extra space after *

  17. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    we should be checking if these are empty strings before adding them to the lists and incrementing the query_items counter.

    something like

    if(split[i]+3 == '\0') {
        continue;
    }
    

    For each block here obviously adjusting the offset as necessary.

  18. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    need to free the lists here too.

  19. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    This should be wrapped... something like.

    g_set_error(error, PURPLE_HISTORY_ADAPTER_DOMAIN, 0,
                _("Error opening database in PurpleSqliteHistoryAdapter "
                  "for file %s"),
                priv->filename);
    
  20. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows us to avoid a line wrap.

  21. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows us to avoid a line wrap.

  22. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    with the previous comment, having a variable of type PurpleSqliteHistoryAdapter avoid this line going past the boundary.

  23. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows us to avoid a line wrap.

  24. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    again, use the variable that the previous comment will create to avoid having a line that goes passed the boundary.

  25. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    We typically create another variable of the type so we don't need to typecast in the _get_instance_private call which allows us to avoid a line wrap.

  26. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    these all should be line wrapped.

  27. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    putting sqlite3_errmsg(priv->db)); on a new line will get this line under 80 chars.

  28. libpurple/purplesqlitehistoryadapter.c (Diff revision 4)
     
     

    You don't need the PURPLE_HISTORY_ADAPTER() here.

  29. libpurple/tests/test_history_adapter.c (Diff revision 4)
     
     

    extra newline

  30. libpurple/tests/test_history_adapter.c (Diff revision 4)
     
     

    Since TestPurpleHistoryAdapter->query returns a list with 1 item you need to free it after the check with g_list_free.

  31. libpurple/tests/test_history_adapter.c (Diff revision 4)
     
     

    uncomment and fix typo.

  32. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    looks like this doesn't need to be wrapped?

  33. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    looks like this doesn't need to be wrapped?

  34. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    should be wrapped when over 80 chars.

  35. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    should be wrapped after 80 chars

  36. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    you can drop the PURPLE_MESSAGE()

  37. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    You can drop the PURPLE_CONVERSATION()

  38. libpurple/tests/test_history_manager.c (Diff revision 4)
     
     

    This should wrap at 80 chars

  39. 
      
grim
grim
grim
rewtguy
grim
  1. 
      
  2. libpurple/purplehistorymanager.h (Diff revision 5)
     
     

    previous and current are PurpleHistoryAdapter's not PurpleHistoryManager's.

  3. libpurple/purplesqlitehistoryadapter.c (Diff revision 5)
     
     

    There should be a space between sqlite3_stmt and the *

  4. libpurple/purplesqlitehistoryadapter.c (Diff revision 5)
     
     

    should be gint

  5. libpurple/purplesqlitehistoryadapter.c (Diff revision 5)
     
     

    there should be a new line before return when it's not the only statement in a block.

  6. libpurple/purplesqlitehistoryadapter.c (Diff revision 5)
     
     

    these three g_list_free_full's can be deleted now as the lists are cleaned up in the loops above and we've transfered ownership of the memory to sqlite.

  7. libpurple/purplesqlitehistoryadapter.c (Diff revision 5)
     
     

    there should not be a space between * and prepared_statement.

  8. 
      
grim
grim
grim
rewtguy
grim
  1. Ship It!
  2. 
      
QuLogic
  1. 
      
  2. libpurple/meson.build (Diff revision 6)
     
     

    Keep math last.

    1. any reason for this?

    2. Mostly convention, I guess.

  3. libpurple/purplehistoryadapter.c (Diff revision 6)
     
     

    Missing the PURPLE_IS_HISTORY_ADAPTER check.

  4. libpurple/purplehistoryadapter.c (Diff revision 6)
     
     

    Missing the PURPlE_IS_HISTORY_ADAPTER check.

  5. libpurple/purplehistorymanager.h (Diff revision 6)
     
     

    Returns:

  6. libpurple/purplehistorymanager.h (Diff revision 6)
     
     

    Returns:

  7. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    Extra space.

  8. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    The old one is deactivated at this point; should it be re-activated?

  9. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    Should be g_set_error_literal

  10. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  11. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    Should this be checking conversation or message, or is that dependent on specific adapters?

  12. libpurple/purplehistorymanager.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  13. libpurple/purplesqlitehistoryadapter.h (Diff revision 6)
     
     

    SQLite belongs in the title?

  14. libpurple/purplesqlitehistoryadapter.h (Diff revision 6)
     
     

    is a class

  15. libpurple/purplesqlitehistoryadapter.h (Diff revision 6)
     
     

    defines inherits?

    1. In the sense that this is a subclass.

    2. OK, but there are two verbs there.

  16. libpurple/purplesqlitehistoryadapter.h (Diff revision 6)
     
     
     

    Does something actually derive from this? It should be final if not.

    1. The intent was to have a subclass that would have a filename of :memory: for people that don't want persistent logs but we didn't get to that as it was a strech goal. I'm not sure if making that a subclass is still a good idea or not as we could just create and both instances of the same class with the different filenames.. Thoughts?

    2. GLib says it's always possible to go final->derivable while not the reverse, so it's better to make it final until a need to change is there.

  17. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    error is leaked.

    1. Uh.. I'm not seeing a leak? Unless error can be set with a non null return.

    2. If bytes == NULL, then the lookup failed and there should be something in error.

    3. Which goes to the caller of the function.

    4. Ah, you're right, this threads way way up to startup, and seems okay.

    5. cool!

  18. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     

    purple_strequal returns TRUE when they're equal; this should not be comparing == 0.

  19. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    This is just split[i][3].

  20. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    split[i][5]

  21. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    split[i][0]

  22. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     

    Since these are all ORd together, it doesn't appear that there's any benefit to putting this in any 'correct' order, so there's no need to reverse the lists.

  23. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should be g_error_set_literal

  24. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     
     

    If query_items is 0, then none of these would have anything in them.

  25. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     

    It would be shorter to write this part of the query as converation_id IN (?, ?, ?, ...).

  26. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    The whitespace probably doesn't matter, but did you intend to put newlines after each type of qualifier?

    1. We did it that way initially for debugging, but I can totally put it back if we want it.

  27. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Same here with author IN (?)

  28. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  29. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     
     

    priv->filename == NULL, so no point in including it in the error message like this, instead of explicitly calling that out.

  30. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    If running migrations fails, then don't you need to close the db handle?

  31. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  32. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     
     
     
     
     
     
     
     
     

    Instead of SELECT *, you probably should have selected these specific columns. Then you don't need to pick random indices like starting from 3 and skipping from 9 up to 13.

  33. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  34. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should you not finalize the prepared statement on error?

  35. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    This is const.

  36. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should be g_set_error_literal.

  37. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Why is this not using the existing message_id?

  38. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    Should you not finalize the prepared statement on error?

  39. libpurple/purplesqlitehistoryadapter.c (Diff revision 6)
     
     

    This needs to clear the db handle (and maybe warn) if it wasn't correctly unregistered.

  40. libpurple/tests/test_history_adapter.c (Diff revision 6)
     
     

    You should pass an error to all these tests and g_assert_no_error.

  41. libpurple/tests/test_history_adapter.c (Diff revision 6)
     
     

    Query is transfer container, not transfer full. Also, the test adapter returns GINT_TO_POINTER(1), so should definitely not be freed.

  42. purple-history/purplehistorycore.c (Diff revision 6)
     
     

    This is unused?

    1. Yes we need to finish determining how we want to handle commands in the the purple-history cli utility. Basically, do we manually check the first argument or use flags like --query foo and --remove bar etc.

    2. Guess that depends on what's generally expected in the GOptionParser stuff.

    3. yeah, i've been having a hard time finding examples is the main issue.

    4. So I dug through gsettings-tool and that doesn't use GOptionParser at all. Can you think of any other glib based tools that have a command based cli?

    5. Looking at the howto and the docs, it doesn't seem like GOptionParser does the non-flag thing.

    6. Yeah so we'll circle back on that in another review request?

  43. purple-history/purplehistorycore.c (Diff revision 6)
     
     

    You aren't checking the return value to exit with the failure return code.

  44. 
      
rewtguy
grim
QuLogic
  1. 
      
  2. libpurple/purplesqlitehistoryadapter.h (Diff revisions 6 - 8)
     
     
  3. libpurple/purplesqlitehistoryadapter.c (Diff revisions 6 - 8)
     
     

    error is already set by g_resource_lookup_data, and error_msg is empty?

  4. libpurple/tests/test_history_adapter.c (Diff revisions 6 - 8)
     
     
     

    Extra line break.

  5. purple-history/purplehistorycore.c (Diff revisions 6 - 8)
     
     

    Should be TRUE now.

  6. purple-history/purplehistorycore.c (Diff revisions 6 - 8)
     
     

    Should be TRUE now.

  7. purple-history/purplehistorycore.c (Diff revisions 6 - 8)
     
     

    Clear error.

  8. 
      
grim
grim
grim
QuLogic
  1. 
      
  2. libpurple/purplehistoryadapter.c (Diff revision 10)
     
     

    This indent seems off.

  3. libpurple/purplehistorymanager.h (Diff revision 10)
     
     

    It seems that adding these annotations for the error is unnecessary since the .gir doesn't not record it directly as a parameter.

    1. so get rid of all of (out) (optional) (nullable)?

    2. Yea, remember that gi-docgen doesn't see this at all, as it's recorded as throws=1, not an actual parameter.

    3. This is also in a couple other files that I didn't note down.

  4. libpurple/purplehistorymanager.c (Diff revision 10)
     
     

    Is this opening in the current directory? It should go to the user config dir.

  5. libpurple/purplehistorymanager.c (Diff revision 10)
     
     

    Can drop the cast.

  6. libpurple/purplehistorymanager.c (Diff revision 10)
     
     

    Should verify that old is not NULL/non-adapter.

  7. libpurple/purplesqlitehistoryadapter.c (Diff revision 10)
     
     
     
     

    Why are there two step calls?

  8. 
      
grim
grim
QuLogic
  1. Ship It!
  2. libpurple/purpleprivate.h (Diff revision 12)
     
     

    Here are the other ones.

  3. 
      
grim
grim
Review request changed

Status: Closed (submitted)

Loading...