GNOME Bugzilla – Bug 705969
[RFC] [WIP] VKontakte provider
Last modified: 2021-07-05 10:59:45 UTC
Today I starts work with support VKontakte API in g-o-a. Documentation: http://vk.com/dev Planned features: * Chats * Contacts * Documents Here I will attach files/patches for new provider and I need helping with developing. If you can help me feel free to comment.
Created attachment 251579 [details] Header file for VKontakte provider
Created attachment 251580 [details] goavkontakteprovider.c
I can see that it uses OAuth 2.0 (http://vk.com/dev/auth_mobile) for authentication. What protocols or APIs are used for chat, contacts and documents?
(In reply to comment #3) > I can see that it uses OAuth 2.0 (http://vk.com/dev/auth_mobile) for > authentication. What protocols or APIs are used for chat, contacts and > documents? I implemented chat for telepathy stack. Chat uses long polling HTTP requests. All is based on HTTP json\xml API. For accesing it needed only access_token from OAuth.
(In reply to comment #0) > Today I starts work with support VKontakte API in g-o-a. > Documentation: http://vk.com/dev > Planned features: > * Chats > * Contacts > * Documents > Here I will attach files/patches for new provider and I need helping with > developing. If you can help me feel free to comment. Напиши мне vk.com/alex_eri
It would be great
Any news?
We should write telepathy CM first. I wrote some things (login, set online/offline status)[0], but I have no time now. Patches are welcome (c) [0]https://github.com/ignatenkobrain/telepathy-durka
*** Bug 744256 has been marked as a duplicate of this bug. ***
Created attachment 297111 [details] [review] vk goa provider vk goa provider. provides only "chat" as of now, which should be switched to "music" at some point.
Created attachment 303845 [details] [review] goa music patch Add support for "music" providers
Created attachment 303846 [details] [review] goa vk patch Add support for the GOA VK provider. It relies on the music interface.
Created attachment 303847 [details] CAPTCHA solving window This is how the CAPTCHA solving is looking right now. The only thing that might be missing is a flashy warning sign "the CAPTCHA you entered was incorrect" or "the CAPTCHA text is empty". Right now in both of these situations the window just silently reloads the CAPTCHA image without any warnings.
Created attachment 304034 [details] CAPTCHA solving window New iteration of the custom dialog.
Created attachment 304079 [details] [review] goa_vk_patch A new iteration of the patch. - Massive rework of the captcha dialog. - Captcha image load now asynchronous. - code style fixes.
Created attachment 304156 [details] [review] goa_vk_patch Quick fix: - fix two objects being double initialized in load_captcha_image () - reintroduce the status_code check which didn't work because of the previous error
Created attachment 305248 [details] [review] goa_vk_patch - MORE async! Now it's as async as one can get. - New simple dialog with a spinner is shown while goa connects to the server - The captcha dialog also shows spinner when connecting. GUI people should be satisfied.
I try apply 2 patch goa_vk_patch and goa_music_patch and compile gnome-obline-accounts-3.16-3. Chat provider not support for me: http://i.imgur.com/s0fhPyU.png
Chat support is not supposed to be there. Not until the TP provider is finished. It's only music for now, and to check it working you need to also rebuild grilo and grilo-plugins with patches from #744238 But what bothers me is why the VK icon is not shown. Are you sure you applied the patch correctly? Do you have the goa-account-vk.png files in your source data folder?
Yes, goa-account-vk.png files exists http://i.imgur.com/HjhVl8n.png
In source directory: http://i.imgur.com/ql3EJuT.png
(In reply to Morse from comment #11) > Created attachment 303845 [details] [review] [review] > goa music patch > > Add support for "music" providers As both this patch and bug 728621 add 'music' feature can we rename this to something like 'VkMusic' instead?
Created attachment 308630 [details] [review] goa_vk_patch Add support for PHOTOS. The patch for rudimentary support for grilo is ready.
Review of attachment 303845 [details] [review]: Thanks for the patch, George. The patch looks good apart from a few minor issues. The commit message can be polished a bit (see commit bb9575cd95cf2 for an example) and the URL of this bug should be added to it. ::: data/dbus-interfaces.xml @@ +187,3 @@ <property name="MapsDisabled" type="b" access="readwrite"/> + <!-- MusicDisabled: Please add the version: @since: 3.18.0 @@ +656,2 @@ <!-- + org.gnome.OnlineAccounts.Music: Ditto.
Review of attachment 308630 [details] [review]: Please add the URL to this bug in the commit message. A few quick comments: ::: configure.ac @@ +297,3 @@ +AC_ARG_ENABLE([vk], [AS_HELP_STRING([--enable-vk], + [Enable VK provider])], + [], Would be nice to align these. @@ +298,3 @@ + [Enable VK provider])], + [], + [enable_vk=yes]) We shouldn't turn it on by default (enable_vk=no) until all the relevant patches are in place. Are photos supported now? ::: data/icons/16x16/Makefile.am @@ +10,3 @@ goa-account-owncloud.png \ goa-account-pocket.png \ + goa-account-vk.png \ Should be aligned. ::: data/icons/22x22/Makefile.am @@ +10,3 @@ goa-account-owncloud.png \ goa-account-pocket.png \ + goa-account-vk.png \ Ditto. ::: data/icons/24x24/Makefile.am @@ +9,3 @@ goa-account-owncloud.png \ goa-account-pocket.png \ + goa-account-vk.png \ Ditto. ::: data/icons/32x32/Makefile.am @@ +10,3 @@ goa-account-owncloud.png \ goa-account-pocket.png \ + goa-account-vk.png \ Ditto. ::: data/icons/48x48/Makefile.am @@ +9,3 @@ goa-account-owncloud.png \ goa-account-pocket.png \ + goa-account-vk.png \ Ditto. ::: src/goabackend/goavkprovider.c @@ +1,2 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* This file is missing copyright information. @@ +102,3 @@ + GoaObject *object) +{ + return g_strdup (_("VK")); Don't you think the full name (ie. VKontakte) will look better in the UI? @@ +115,3 @@ +{ + return GOA_PROVIDER_FEATURE_BRANDED | + /* GOA_PROVIDER_FEATURE_CHAT | */ What is the status of chat? My impression is that it is not going to usable in the next few weeks. In that case, please remove the commented line. We can add it later when it is ready. @@ +159,3 @@ +{ + return + /* "messages," */ /* to use the chat */ Ditto. @@ +160,3 @@ + return + /* "messages," */ /* to use the chat */ + "friends," /* to see the list of friends (contact list) */ Do we need "friends" without chat? @@ +162,3 @@ + "friends," /* to see the list of friends (contact list) */ + "audio," /* to play the music */ + "offline"; /* to be able to use API even when user is not logged into the website */ Doesn't photos need some scope(s)? @@ +169,3 @@ +{ + return 1; +} You don't need to implement it. The default implementation will return 0 which should be OK until you change the scopes. ::: src/goabackend/goavkprovider.h @@ +1,2 @@ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ +/* This file is missing copyright information.
Review of attachment 308630 [details] [review]: Some more comments: ::: src/goabackend/goavkprovider.c @@ +36,3 @@ + * + * The #GoaVkProvider structure contains only private data and should + * only be accessed using the provided API. Typo: GoaVKProvider, not GoaVkProvider. :) @@ +57,3 @@ + VK_TOKEN_CAPTCHA_NEEDED, + VK_TOKEN_INVALID +} VKTokenResult; How about calling it VKTokenStatus? Like SoupStatus. @@ +139,3 @@ + escaped_redirect_uri, + escaped_client_id, + escaped_scope); Broken alignment. @@ +215,3 @@ + gchar *debug_str; + + if (!JSON_NODE_HOLDS_OBJECT(error_node)) { We put the opening brace in the next line. See some of the other files for examples. @@ +230,3 @@ + case 14: /* captcha required */ + if (json_object_has_member (error_obj, "captcha_img") && json_object_has_member (error_obj, "captcha_sid")) { + if (vk_captcha != NULL) { Ditto. @@ +247,3 @@ + case 5: /* token access revoked */ + case 7: /* wrong app priveleges */ + case 17: /* validation needed */ How about using an enumerated type for all the error codes? That would be inherently readable you won't have to add a comment every time one of the integers are used. @@ +269,3 @@ + if (request_params) { + /* As tokens are private, we should remove it from the list of parameters before printing */ + for (int i = 0; i < json_array_get_length (request_params); i++) { Please move the 'int i' to the beginning of the block. @@ +401,3 @@ + presentation_identity = g_strdup_printf ("%s %s", + json_object_get_string_member (json_object, "first_name"), + json_object_get_string_member (json_object, "last_name")); Broken alignment. @@ +606,3 @@ + GoaObject *object, + GtkWindow *parent, + GError **error) None of the other virtual function implementations use the goa_vk_provider prefix. Why not just call it refresh account? @@ +608,3 @@ + GError **error) +{ + GoaVKProvider *vk_provider = GOA_VK_PROVIDER (provider); It should be 'self', not 'vk_provider'. @@ +999,3 @@ + GtkGrid *grid, + G_GNUC_UNUSED GtkGrid *dummy) +{ With the changes in bug 752941 now in master, you don't need to implement this function anymore.
Adding ui-review because the CAPTCHA dialog (attachment 304034 [details]) needs design input.
Comment on attachment 251579 [details] Header file for VKontakte provider Obsoleted by the new patches from George.
Comment on attachment 251580 [details] goavkontakteprovider.c Obsoleted by the new patches from George.
Review of attachment 303845 [details] [review]: You also need to update the show_account_items array in goaprovider.c. I prefer adding a music entry just before photos.
Created attachment 308750 [details] [review] goa vk patch >Please add the version: >@since: 3.18.0 Done >Would be nice to align these. >We shouldn't turn it on by default (enable_vk=no) until all the relevant patches are in place. Done and done. >Are photos supported now? I wrote the rudimentary support for grilo: the "browse" function shows all of the photos of the user. Tested it with grilo-test-ui, and it worked, but I didn't find any real-life apps to test it with. Unfortunately it needs some patches for lua-factory first (same with music). >Should be aligned. Tabs don't make it easy. I hope it's done (my tabs are set to 2 spaces). >This file is missing copyright information. Done >Don't you think the full name (ie. VKontakte) will look better in the UI? "VK" _is_ it's full name. In english. In russian it's "ВКонтакте", but that's for translators to care. >What is the status of chat? Uncertain. Removed. >Do we need "friends" without chat? I use the list of friends to show their music, and plan to do the same with photos. I read some feedback on my VK Music plugin for rhythmbox and found out that that was a requested feature. >Doesn't photos need some scope(s)? Hm, strange thing it worked without it. Done. >You don't need to implement it. Removed. >This file is missing copyright information. Done >Typo: GoaVKProvider, not GoaVkProvider. :) Done >How about calling it VKTokenStatus? Like SoupStatus. Done >Broken alignment. These tabs again. Done. >We put the opening brace in the next line. See some of the other files for examples. I did actually. I stole most of the code from facebook to begin with. grep ") {" shows 28 results throughout GOA :). Anyway, done. >How about using an enumerated type for all the error codes? That would be inherently readable you won't have to add a comment every time one of the integers are used. Actually, they are used just once. I specifically made this error-handling function to not use the numbers anywhere else. So I don't really see how the enum will be easier. VK API has ~100 different errors. I only picked the ones I care about. Making the enum, I should add them all. I really do not want to do it. >Please move the 'int i' to the beginning of the block. Done. It's a shame glib json arrays do not have some handy next() mechanism. >Broken alignment. No tabs this time. Strange. Done. >None of the other virtual function implementations use the goa_vk_provider prefix. Why not just call it refresh account? Done. >It should be 'self', not 'vk_provider'. glib so much wants to be C++ :). Done. >With the changes in bug 752941 now in master, you don't need to implement this function anymore. Done. Not tested though (I'll be able to fire up my build machine only on weekend).
Created attachment 308751 [details] [review] goa music patch >The commit message can be polished a bit (see commit bb9575cd95cf2 for an example) and the URL of this bug should be added to it. Done. >You also need to update the show_account_items array in goaprovider.c. I prefer adding a music entry just before photos. Done.
>Adding ui-review because the CAPTCHA dialog (attachment 304034 [details]) needs design input. I modified it a little since that screenshot: I added the label "Enter the text from the image below" on top (as it was suggested on #gnome-design).
Review of attachment 308750 [details] [review]: ::: data/dbus-interfaces.xml @@ +189,2 @@ <!-- MusicDisabled: + @since: 3.18.0 This should be in the other patch. @@ +658,3 @@ <!-- org.gnome.OnlineAccounts.Music: + @since: 3.18.0 Ditto.
Review of attachment 308751 [details] [review]: Still missing the version information.
Created attachment 308791 [details] [review] Introduce org.gnome.OnlineAccounts.Music I took the liberty to fix it up because bug 728621 depends on it.
Created attachment 308792 [details] [review] goa_vk_patch Since you already fixed the first patch, I'll only upload the second.
*** Bug 779375 has been marked as a duplicate of this bug. ***
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/issues/ Thank you for your understanding and your help.