GNOME Bugzilla – Bug 673600
Add meaning feature to evince
Last modified: 2018-05-22 14:33:22 UTC
We think it would be a nice enhancement if there was a feature that allowed a user to look up the meaning of a word directly from within evince -- using an online resource..
Created attachment 211426 [details] [review] Patch for Bug 673600 We have added the mentioned enhancement to edit menu of evince The user needs to have libcurl installed for this to work
While I would like to see a feature like this in evince (or even more generally in any gtk application) this patch needs a lot of work. You need to follow the code style of evince source code, just for starter. You also would need to pack your widgets in a new one that you construct in a new file, something like ev-word-meaning.c and encapsulate all the logic there. Also, you can't just assume that libcurl is available, you need to explicitly add the dependencie checks in the configure.ac... also be careful with what you include in your patch, for instance, You are not modifying configure.ac, yet you included it into your patch. Finally, always produce patches against git master. If you need more help, feel free to ping me on irc.
Created attachment 211452 [details] [review] Modified patch for Bug 673600 Modified patch for Bug 673600 Changes to previous patch as requested in comment
Review of attachment 211452 [details] [review]: In general, please don't attach a patch that fixes your last patch, but attach a patch that includes both your first patch and the corrections for it. ::: configure.ac @@ +195,3 @@ PKG_CHECK_MODULES([SHELL_CORE],[libxml-2.0 >= $LIBXML_REQUIRED gtk+-3.0 >= $GTK_REQUIRED gio-2.0 >= $GLIB_REQUIRED gthread-2.0 libcurl >= 7.21.6 $SHELL_PLATFORM_PKGS]) +PKG_CHECK_EXISTS([libcurl >= 7.21.6], [echo 'libcurl found'], [echo 'libcurl was not found please use apt-get install libcurl to get it']) apt-get exists for the Debian distribution and its variants. RPM-based distributions do not have apt-get. Please do not mention apt-get here.
Created attachment 211455 [details] [review] Updated patch for Bug 673600 Contains both the previous patch followed by the new one Removed the term "apt-get" from the configure.ac file
Review of attachment 211455 [details] [review]: Please describe in the Subject line of the patch the patch. I don't care about "We have"s etc or every single file - I can see that myself in the git log. Instead provide a summary of what the patch does. Same problems as before. ::: configure.ac @@ +195,3 @@ PKG_CHECK_MODULES([SHELL_CORE],[libxml-2.0 >= $LIBXML_REQUIRED gtk+-3.0 >= $GTK_REQUIRED gio-2.0 >= $GLIB_REQUIRED gthread-2.0 libcurl >= 7.21.6 $SHELL_PLATFORM_PKGS]) +PKG_CHECK_EXISTS([libcurl >= 7.21.6], [echo 'libcurl found'], [echo 'libcurl was not found please use apt-get install libcurl to get it']) apt-get. As usual. I will not continue reviewing this patch as the previous review comments were not implemented.
Created attachment 211459 [details] [review] Updated patch for Bug 673600 Changed removed "apt-get"
(In reply to comment #7) > Created an attachment (id=211459) [details] [review] > Updated patch for Bug 673600 That's not an updated patch. It's only a patch for the patch before and I've written before that we'd like to have ONE patch only, and not patches for patches for patches for patches...
Plus for "libcurl was not found or is old please install libcurl" you likely miss punctuation. Plus you likely don't need to isntall libcurl for this, but the devel package for it to get its header file, so it's misleading.
Plus "libcurl was not found or is old please install libcurl" does not make sense. In case it "is old" why should I install it? It already is installed. And what does "old" mean? And how would I find out what is not "old" and needed?
Created attachment 211464 [details] [review] Updated patch for Bug 673600 Updated patch
Review of attachment 211464 [details] [review]: As José said, this patch needs a lot of work. Feel free to ask here or on IRC if you need help. ::: configure.ac @@ +195,3 @@ PKG_CHECK_MODULES([SHELL_CORE],[libxml-2.0 >= $LIBXML_REQUIRED gtk+-3.0 >= $GTK_REQUIRED gio-2.0 >= $GLIB_REQUIRED gthread-2.0 libcurl >= 7.21.6 $SHELL_PLATFORM_PKGS]) +PKG_CHECK_EXISTS([libcurl >= 7.21.6], [echo 'libcurl found'], [echo 'libcurl version 7.21.6 or higher is required. Please install the package libcurl-openssl-dev version 7.21.6 or higher.']) I prefer to use libsoup, which is the gnome http library, instead of using libcurl ::: shell/ev-find-meaning.c @@ +4,3 @@ + * + * Finds the meaning of a word from the internet and displays it + */ You should include the license text in addition to the authors. See other files in the same directory. @@ +11,3 @@ +void ev_find_meaning() +{ +meaning_window=gtk_window_new(GTK_WINDOW_TOPLEVEL); This function is not indented at all. @@ +13,3 @@ +meaning_window=gtk_window_new(GTK_WINDOW_TOPLEVEL); + +gtk_window_set_title(GTK_WINDOW(meaning_window),"MEANING:"); That should be marked for translation and not in capitals @@ +18,3 @@ +grid=gtk_grid_new(); + +button=gtk_button_new_with_label("Search"); This should be marked for translation too. @@ +20,3 @@ +button=gtk_button_new_with_label("Search"); + +text_find=gtk_entry_new(); Instead of using an entry where you have to type the word to search, it would be better to use the currently selected work. @@ +24,3 @@ +gtk_grid_attach(GTK_GRID(grid),button,1,0,1,1); + +g_signal_connect(button,"clicked",G_CALLBACK(testfunc),NULL); testfunc? it doesn't sound like a valid name for a final patch @@ +54,3 @@ + gpointer args; + GThread *thread1; + thread1=g_thread_create((GThreadFunc)testdialog1,args,TRUE,NULL); testdialog1? why using a thread? @@ +114,3 @@ + + website = (char *)malloc(1000); + sprintf(website,"http://freedictionary.org/?Query=%s",text_value); I wonder if we could use the gnome-dictionary API, there's a libgdict, in gnome-dictionary, but I don't know whether it's public API or not. ::: shell/ev-find-meaning.h @@ +29,3 @@ +static GtkTextBuffer *buffer; +GtkWidget *meaning_window,*button,*grid; +GtkWidget *meaning_scroll,*vbox; Don't use global variables.
Created attachment 212512 [details] [review] Updated patch
Created attachment 212517 [details] [review] Updated patch this is the new patch that contains all the changes.
Sorry about patch 212512. It was incomplete due to some problem with my local git copy. Our sincere apologies for any inconvenience. Therefore kindly consider the new patch (212517) for review
Review of attachment 212517 [details] [review]: > Subject: [PATCH] should contain a description of the patch, not duplicated information such as modified files or authors. That's all in the metadata already. Did you consider using libgdict as mentioned in a previous review? If no, why not? If yes, what was the result? ::: configure.ac @@ +185,3 @@ +# LIBSOUP for meaning plugin in ev-window.c + +PKG_CHECK_EXISTS([libsoup-gnome2.4 >= 2.36], [echo 'libsoup found'], [echo 'libsoup was not found or is old. Please install the package libsoup-gnome2.4-dev version 2.36 or later']) Package names are different on each distribution, so I'd say "libsoup" only. @@ +186,3 @@ + +PKG_CHECK_EXISTS([libsoup-gnome2.4 >= 2.36], [echo 'libsoup found'], [echo 'libsoup was not found or is old. Please install the package libsoup-gnome2.4-dev version 2.36 or later']) +PKG_CHECK_MODULES(LIBSOUP, [libsoup-gnome-2.4 >= 2.36]) This won't work globally as this name is likely only used by your distribution. ::: data/evince-ui.xml @@ +29,3 @@ <menuitem name="EditRotateRightMenu" action="EditRotateRight"/> <separator/> + <menuitem name="EditMeaning" action="EditMeaning"/> Remove the useless indentation for those two lines. ::: libview/ev-view.c @@ +6455,3 @@ } + +/*Edited by Codedemons*/ Please remove such comments. @@ +6456,3 @@ + +/*Edited by Codedemons*/ +char * ev_find_meaning_get_selected_text(EvView *view) This line violates the style of any other function definitions in that file: return value in the same line, missing whitespaces. @@ +6460,3 @@ + return get_selected_text(view); +} + Useless whitespace, please clean up first and go through your patch yourself before attaching it here. ::: libview/ev-view.h @@ +111,3 @@ void ev_view_cancel_add_annotation (EvView *view); +/* Edited by Codedemons*/ Please remove such comments. ::: shell/ev-find-meaning.c @@ +30,3 @@ +void ev_find_meaning(EvView *view) +{ + //g_type_init(); Please clean up the code first and remove all // commented lines before attaching it. @@ +86,3 @@ + +} +char * meaning_soup_fetcher(char *website) { Whitespace between * and function name, you don't have that for ev_find_meaning_worker(). @@ +123,3 @@ + struct meaning_widgets *wid; + wid=(struct meaning_widgets *)data; + const char *text_value; In general: Is there a reason why you don't use gchar/guchar instead of char? See http://developer.gnome.org/glib/stable/glib-Basic-Types.html @@ +126,3 @@ + text_value=gtk_entry_get_text(GTK_ENTRY(wid->text_find)); + char *website,res1[100][1000],*web_data,no[5]; + int pos=0,i=0,error=0; Same question for guint/gint vs int... @@ +128,3 @@ + int pos=0,i=0,error=0; + + website = (char *)malloc(1000); I might be wrong here as I don't code, but shouldn't website also get initialized? Why did you choose a length of 1000? @@ +135,3 @@ + if(!web_data) { + gtk_window_resize(GTK_WINDOW(wid->meaning_window),320,240); + gtk_text_buffer_set_text (wid->buffer,_("Please connect to the internet..."),-1); Is a missing connection to the internet the only possible reason for !web_data ? ::: shell/ev-window.c @@ +5809,2 @@ /* Edit menu */ + { "EditMeaning",GTK_STOCK_INFO,N_("_Meaning"),"<control>M",N_("Find word Meaning"),G_CALLBACK(ev_find_meaning_cb) },/*TUMUL*/ Capitalization in "Find word Meaning" is inconsistent. What is "TUMUL"? @@ +7619,3 @@ } + +/*Edited by Codedemons*/ Please remove the comment.
Did you consider using libgdict as mentioned in a previous review? If no, why not? If yes, what was the result? We didnt use libgdict because it has far more dependencies than libsoup so it will increase the size of evince considerably. However the meanings that are fetched are both from the internet and do not differ a lot. +PKG_CHECK_MODULES(LIBSOUP, [libsoup-gnome-2.4 >= 2.36]) We have now included support for both libsoup and libsoup-gnome.We didnt remove libsoup-gnome because it provides proxy support for gnome users. I might be wrong here as I don't code, but shouldn't website also get initialized?Why did you choose a length of 1000? However large the word might be the length of the url(which the variable contains) will not exceed 1000 Rest of the changes have been made
Created attachment 212566 [details] [review] Updated patch
Thanks, the patch looks better! You seem to have it based on an old version though, so you introduce lots of stuff that was removed in evince, e.g. you change 3.4 back to 3.3 again and also introduce hildon support again. Please rebase the patch. (In reply to comment #17) > +PKG_CHECK_MODULES(LIBSOUP, [libsoup-gnome-2.4 >= 2.36]) > > We have now included support for both libsoup and libsoup-gnome.We didnt remove > libsoup-gnome because it provides proxy support for gnome users. Maybe this is the case for your distibution, but not anywhere else. On a Fedora 16 machine: $:andre\> rpm -qa | grep libsoup libsoup-2.36.1-2.fc16.i686 libsoup-devel-2.36.1-2.fc16.i686 libsoup-debuginfo-2.36.1-2.fc16.i686 That's all. So it still will not work on any other distribution. > I might be wrong here as I don't code, but shouldn't website also get > initialized?Why did you choose a length of 1000? > > However large the word might be the length of the url(which the variable > contains) will not exceed 1000 That only answers my second question, but not the first one. :)
I might be wrong here as I don't code, but shouldn't website also get initialized?Why did you choose a length of 1000? To answer your first question please note that website is the string that contains the url. It is dynamically allocated whenever we enter the function and we are using sprintf to put the required url into website(variable) and we free it at the end of the function. Therefore, we dont have to initialize it. For the second question, we now allocate exactly the amount of space required in the website variable ie length of the word in textbox(text_value) + 50 char for the remaining part of the url that remains constant for every query We have changed the error message from libsoup-2.4 to libsoup We have made the patch on evince-3.4
Created attachment 212588 [details] [review] Updated patch
Review of attachment 212588 [details] [review]: Thanks for the patch, it looks indeed better. I still think we should consider using libgdict or even gnome-dictionary directly since it has command line arguments to lookup a word. ::: configure.ac @@ +186,3 @@ + +PKG_CHECK_EXISTS([libsoup-gnome-2.4 >= 2.36], [echo 'libsoup-gnome found'], [echo 'libsoup-gnome was not found or is old.']) +PKG_CHECK_EXISTS([libsoup-2.4 >= 2.36], [echo 'libsoup found'], [echo 'libsoup was not found or is old. Please install the package libsoup version 2.36 or later']) PKG_CHECK_MODULES already checks whether it exists or not, so we don't need these. @@ +189,3 @@ + +PKG_CHECK_MODULES(LIBSOUP_GNOME, + [libsoup-gnome-2.4 >= 2.26], why libsoup-gnome and not just libsoup, what do you need from libsoup-gnome? ::: data/evince-ui.xml @@ +29,3 @@ <menuitem name="EditRotateRightMenu" action="EditRotateRight"/> <separator/> + <menuitem name="EditMeaning" action="EditMeaning"/> This should not be in Edit menu, since it doesn't affect the document at all. Maybe in View. You should also add an entry to the context menu available when there's selected text. ::: libview/ev-view.c @@ +6456,3 @@ + +gchar * +ev_find_meaning_get_selected_text(EvView *view) This method is not specific to the meaning feature, it returns the text currently selected in EvView. Just rename get_selected_text to ev_view_get_selected_text() and make it public. Remember to add gtk-doc comments, since this will be part of the public API. ::: shell/Makefile.am @@ +23,3 @@ $(DISABLE_DEPRECATED) +AM_CPPFLAGS = ${LIBSOUP_CFLAGS} This should be part of INCLUDES and use () instead of {} @@ +119,3 @@ $(top_builddir)/libmisc/libevmisc.la \ + $(SHELL_LIBS) \ + ${LIBSOUP_LIBS} You should use () instead {} ::: shell/ev-find-meaning.c @@ +3,3 @@ + * Srijan Mukherjee <code.demons@yahoo.com> + * + * Finds the meaning of a word from the internet and displays it Add copyright information in addition to the authors. @@ +20,3 @@ + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + This file doesn't follow the coding style at all, please look at other files and try to follow the same coding style @@ +23,3 @@ +#include <glib/gi18n.h> +#ifdef HAVE_CONFIG_H +#include "config.h" Move config include at the beginning. @@ +34,3 @@ + struct meaning_widgets *wid; + wid=(struct meaning_widgets *)malloc(sizeof(struct meaning_widgets)); + wid->meaning_window=gtk_window_new(GTK_WINDOW_TOPLEVEL); This file should implement a new GtkWidget, probably inheriting from GtkWindow, instead of defining an struct with the widgets it contains. See ev-annotation-properties-dialog.[ch] for an example. @@ +41,3 @@ + grid=gtk_grid_new(); + + button=gtk_button_new_with_label(_("Search")); The work should be looked up automatically, no need for a Search button. @@ +47,3 @@ + selected_word=ev_find_get_selected_text(view); + + if(selected_word!=NULL) We should only allow to look up selected text, otherwise you should use the gnome-dictionary or the dictionary application of the platform evince is running. @@ +99,3 @@ + + #ifdef HAVE_LIBSOUP_GNOME + session = soup_session_async_new_with_options(SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_GNOME_FEATURES_2_26, what do you need from SOUP_TYPE_GNOME_FEATURES_2_26? @@ +109,3 @@ + + msg = soup_message_new(SOUP_METHOD_GET,website); + soup_session_send_message (session, msg); This is synchronous and blocks the UI, you should use the async API of libsoup to make sure the UI is not blocked. @@ +141,3 @@ + if(!web_data) { + gtk_window_resize(GTK_WINDOW(wid->meaning_window),320,240); + gtk_text_buffer_set_text (wid->buffer,_("Please connect to the internet..."),-1); The operation can fail for a lot of reasons, not just because there's no internet connection, you should handle the http response.
*** Bug 650051 has been marked as a duplicate of this bug. ***
Libsoup gnome has been used so that we have proxy settings support. While reading stuff people might wish to look up meanings of words that may not be present in the pdf (for example a related word to something they have already looked up the meaning) Hence both the options for automatically taking the selected text as well as allowing the user to type the word has been given. SOUP_TYPE_GNOME_FEATURES_2_26 This automatically adds the SOUP_TYPE_PROXY_RESOLVER which we need. Sorry for the late response.. We were busy with exams and academic projects. The revised patch is attached.
Created attachment 221696 [details] [review] Revised patch Required changes as per previous review have been made.
Comment on attachment 221696 [details] [review] Revised patch >Subject: [PATCH] Revised patch for meaning feature > GObject support introduced. Asynchronous API of libsoup is used. The menu entry has been moved from Edit menu to View Menu. This is nice to know as a comment in Bugzilla, but the difference between patch version 14 and patch version 15 only should not be in the commit message. The commit message should describe what the patch does. There is lots of inconsistency with regard to using whitespace, for example (but ont only): function_name(parameter) vs function_name (parameter). This should be consistent with the surrounding code. Also, is this a complete patch, or just a patch for the previous patch? We'd prefer a complete patch to review here.
Created attachment 221785 [details] [review] Complete patch for find meaning feature Attached is the complete patch for the find meaning feature. Fixed the whitespace inconsistency issue within the source code. And also provided a description of what the patch does instead of highlighting the changes...
[jhbuild]$:andre\> git am 0001-Find-Meaning-feature-for-evince.patch Applying: Find Meaning feature for evince /home/andre/git-gnome/evince/.git/rebase-apply/patch:99: trailing whitespace. gchar * /home/andre/git-gnome/evince/.git/rebase-apply/patch:103: trailing whitespace. } /home/andre/git-gnome/evince/.git/rebase-apply/patch:188: trailing whitespace. /home/andre/git-gnome/evince/.git/rebase-apply/patch:211: trailing whitespace. /home/andre/git-gnome/evince/.git/rebase-apply/patch:222: trailing whitespace. warning: squelched 31 whitespace errors warning: 36 lines add whitespace errors. [jhbuild]$:andre\>
Created attachment 221823 [details] [review] Complete patch for find meaning feature We have fixed trailing whitespace and other space before tab errors and verified by applying the patch ourselves. Hope there are no more problems. The new patch is attached.
Not a review, just some comments: I find the use of stack-allocated buffers disturbing. The 100k bytes allocated for res1 in ev_find_meaning_gotdata() is a large chunk of stack. And, large as it is, it could be overflowed in meaning_soup_extract(), if the argument s contained a string matching the regex s..n.class="definition". not followed by a '<'. So as it stands, it's a huge exploit hole. I didn't look at the other stack-allocated buffers. Compilation with -D_FORTIFY_SOURCE=2 would presumably find all the problematic cases. They could all be eliminated by rewriting everything to use glib heap-based allocation. For intstance, in meaning_soup_extract(), use GPtrArray to create an NULL-terminated array of pointers to NULL-terminated strings instead of populating the limited array res1.
Comment on attachment 221823 [details] [review] Complete patch for find meaning feature Setting patch status accordingly (esp. comment 30). I think the whole approach is wrong; shouldn't try to parse 'html' with a homegrown parser, but instead use a service with a welldefined API (whether returning xml or json or whatever) that will be parsed with an established API (libxml2, json-glib).
Yes, I proposed to reuse gnome-dictionary somehow if possible instead of implementing our own dictionary service.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/275.