GNOME Bugzilla – Bug 698059
Table of Content plugin
Last modified: 2013-05-04 08:20:07 UTC
Created attachment 241562 [details] [review] gnote-0.7.6_tableofcontent-0.2.patch This is a port of the Table of Content plugin that I wrote earlier for Tomboy: http://oluc.blogspot.fi/2011/04/tomboy-add-in-table-of-content.html See the patch enclosed. It is mostly complete, but 1. It is ported against gnote-0.7.6, because this is what I have on my current machine. I will port it to 3.8/GTK3 next. 2. There is a segfault which appears "randomly". To repeat it, just keep showing and hiding the table of content in the plugin menu. It crashes sometimes with the message "(gnote:5992): GLib-GObject-CRITICAL **: g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed", but sometimes with no message at all (just "Segmentation fault"). This must be fixed. I would need some help to track it down (what tools/method?). 3. In general I am uncertain with C++ memory management, so there might be some errors/leaks. A good code review would be welcome. 4. I did not manage to get the 'Level' enum used by both .cpp files (?!?). So I used constants repeated in each file (...). There are scope issues. Adding #include "tableofcontentnoteaddin.hpp" in tableofcontentmenuitem.cpp complains that the module is called two times. Any hint? Bonus, in addition to the features of the Tomboy add-in, this port is also showing the table of content in the contextual menu, and also in a popup menu (on Ctrl-Alt-1). This is a feature that I has on my local Tomboy, but I did not took time to publish it yet.
Review of attachment 241562 [details] [review]: I haven't compiled nor tried the patch. But I put in a few comments. Looks very promising. ::: src/addins/tableofcontent/tableofcontentmenuitem.cpp @@ +35,3 @@ + const std::string & header, + const /*Level::Type*/ int header_level, + const int header_position) const here is not needed. They are copied anyway @@ +36,3 @@ + const /*Level::Type*/ int header_level, + const int header_position) + //: Gtk::ImageMenuItem(header) //FIXME: needed at all? it should be or it will call the default constructor for the perant class @@ +39,3 @@ +{ + m_note = note; + m_header_position = header_position; Please use the initializer list in the constructor instead of assigments @@ +66,3 @@ + set_label("└→ " + header); + } + else { /* do nothing */ } remove the else. @@ +72,3 @@ +void TableofcontentMenuItem::on_activate() +{ + if (!m_note) { return; } indentation ::: src/addins/tableofcontent/tableofcontentmenuitem.hpp @@ +51,3 @@ +static const int Level_H2 = 2; +static const int Level_H3 = 3; +static const int Level_None = 0; You could do typedef enum { H1, H2, H3, None } Level; somewhere ::: src/addins/tableofcontent/tableofcontentnoteaddin.cpp @@ +222,3 @@ + else return Level_None; + else return Level_None; +} indentation if () statement; else statement; @@ +286,3 @@ + return true; + } + else { return false; } indentation @@ +295,3 @@ + return true; + } + else { return false; } indentation @@ +333,3 @@ + buffer->set_active_tag ( (header_request == Level_H2)?"size:huge":"size:large"); + } + else {/*nothing*/} then remove the "else" ::: src/addins/tableofcontent/tableofcontentnoteaddin.hpp @@ +56,3 @@ +const int Level_H2 = 2; +const int Level_H3 = 3; +const int Level_None = 0; Make this an enum in the Addin class and use that in the menu item class. @@ +95,3 @@ + m_tag_bold, + m_tag_large, + m_tag_huge; It is nicer to write this as Type member1; Type member2; Type member3;
(In reply to comment #0) > 2. There is a segfault which appears "randomly". To repeat it, just keep > showing and hiding the table of content in the plugin menu. It crashes > sometimes with the message "(gnote:5992): GLib-GObject-CRITICAL **: > g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed", but sometimes > with no message at all (just "Segmentation fault"). This must be fixed. I would > need some help to track it down (what tools/method?). Looks like an attempt to access an object that has been released. valgrind is usually good at finding this. > 4. I did not manage to get the 'Level' enum used by both .cpp files (?!?). So I > used constants repeated in each file (...). There are scope issues. Adding > #include "tableofcontentnoteaddin.hpp" in tableofcontentmenuitem.cpp complains > that the module is called two times. Any hint? I would just create a tableofcontent.hpp or something like that for the data model that is common. The error you indicate shouldn't happen, but I would have to actually compile to it see it.
You really should upgrade to most recent version and work on it. Otherwise it looks promising, so keep on. A couple of points to note: - add copyright to add-in (override copyright() in this version, use apropriate .desktop file entry in most recent version) - update POTFILES.in with new files (run `intltool-update -m` from po directory) - comment are good in the code, but don't put in too much :) Looking forward for update!
Thank you Hubert and Aurimas for the review! (In reply to comment #1) > + const int header_position) > + //: Gtk::ImageMenuItem(header) //FIXME: needed at all? > > it should be or it will call the default constructor for the perant class Here I think that the default constructor is good. I do not want to set the Label text at this point, as I set it later in the function depending on the header level. I applied all the other changes. I used a tableofcontent.hpp file to host the enum. It works like this and is much cleaner than the duplicated constants. (I do not know why I can't put the enum in tableofcontentnoteaddin.hpp). (In reply to comment #3) > You really should upgrade to most recent version and work on it. Yes I will. I am somehow blocked in 2011 with GNOME 2, which works well for me. GNOME 3.0 has been a strong regression for me. I test it regularly because working with an outdated distribution is a pain. 3.8 starts to be more acceptable. But that's another discussion. Once the plugin works on 0.7.x, I'll port it to 3.8. It was just easier to get started with. > Otherwise it looks promising, so keep on. > > A couple of points to note: > - add copyright to add-in (override copyright() in this version, use done. > apropriate > .desktop file entry in most recent version) > - update POTFILES.in with new files (run `intltool-update -m` from po > directory) Do you want me to provide the po update in the patch? > - comment are good in the code, but don't put in too much :) OK. I need a minimum of comments to know what I write, because reading pure code is not straight forward for me, especially this C++/gtkmm syntax is very heavy to me. My last experience with C++ was at school 20 years ago. I am not really a developer. I learned how to write programs as part of my mathematical studies, then I specialized in software engineering (that is, everything else but the code). But as a side effect I can sometimes produce small pieces of software. I attach the updated patch
Created attachment 241595 [details] [review] gnote-0.7.6_tableofcontent-0.3.patch
(In reply to comment #2) > (In reply to comment #0) > > 2. There is a segfault which appears "randomly". To repeat it, just keep > > showing and hiding the table of content in the plugin menu. It crashes > > sometimes with the message "(gnote:5992): GLib-GObject-CRITICAL **: > > g_object_get_qdata: assertion `G_IS_OBJECT (object)' failed", but sometimes > > with no message at all (just "Segmentation fault"). This must be fixed. I would > > need some help to track it down (what tools/method?). > > Looks like an attempt to access an object that has been released. > > valgrind is usually good at finding this. About the segfault, I noticed that it already exists in upstream gnote, in the backlinks plugin (which I used as a template). To reproduce open a note (possibly with backlinks), and hover across the plugin menu to open and close the backlinks submenu, after a few times, it crashes. Is it a known bug? Could someone check if this is also present in more recent gnote, or if it was fixed? I attach the output of valgrind. It is the first time I run valgrind. This is a bit Chinese for me. Maybe you understand better?
Created attachment 241596 [details] gnote-crash-output.txt this is a portion of valgrind output, related to the segfault bug
Created attachment 241635 [details] [review] gnote-0.7.6_tableofcontent-0.4.patch I applied the same change as in https://github.com/GNOME/gnote/commit/14cea93282d9aae279d2405a141d8a941236b2c4 I think it fixes the segfault. See updated patch. Next step: porting to gnote-3.8
Review of attachment 241635 [details] [review]: Few more minor changes. If time permits, I might even try to port it to most recent gnote myself. ::: src/addins/tableofcontent/tableofcontent.hpp @@ +25,3 @@ +namespace tableofcontent { + +namespace Level { enum Type {H1, H2, H3, None}; }; // H1=title, H2:bold/huge, H3:bold/large I suggest to name enum elements LEVEL_H1, LEVEL_H2... and remove the namespace. Format enum so that each item is on a separate line and put comments next to them. ::: src/addins/tableofcontent/tableofcontentnoteaddin.cpp @@ +67,3 @@ +const char * TableofcontentModule::copyright() const +{ + return "Luc Pionchon"; Append \xc2\xa9 character and year here. @@ +92,3 @@ +{ + // TOC menu + m_menu = manage(new Gtk::Menu()); Two spaces indentation instead of tab. @@ +152,3 @@ + } + //NOTE: Gtk::Menu is a Gtk::Container which has a foreach function + //void Gtk::Container::foreach(const ForeachSlot& slot) What is the point of this comment? @@ +233,3 @@ + return Level::None; + else + return Level::None; Add braces for all ifs and elses. @@ +301,3 @@ + { + return false; + } Put opening brace at the end of above lines for all if/else here. @@ +313,3 @@ + { + return false; + } Like above, opening brace at the end of line, not on new line. @@ +350,3 @@ + buffer->set_active_tag ("bold"); + buffer->set_active_tag ( (header_request == Level::H2)?"size:huge":"size:large"); + } Opening brace at the end of line, not new line.
Created attachment 241681 [details] [review] gnote-3.6.0_tableofcontent-0.5.patch This is a minor update to work with gnote-3.6.0. I tried to compile it against gnote master, but my GNOME3 partition (ubuntu quantal) has gtkmm-3.5 and gnote master requires gtkmm-3.6. I am not sure how I could upgrade safely the libraries? Could someone try it with gnote master? At this stage, maybe I could upload a binary somewhere for volunteers to test it further? Would it be enough to provide tableofcontent.so, and drop it in ~/.config/gnote/addins/ ?
(ah, I did not see you latest review, I will fix it, thanks!)
(In reply to comment #9) > ::: src/addins/tableofcontent/tableofcontent.hpp > @@ +25,3 @@ > +namespace tableofcontent { > + > +namespace Level { enum Type {H1, H2, H3, None}; }; // H1=title, H2:bold/huge, > H3:bold/large > > I suggest to name enum elements LEVEL_H1, LEVEL_H2... and remove the namespace. > Format enum so that each item is on a separate line and put comments next to > them. When using just enum (and prefixed elements), it can't be used as a type. The compiler complains the object not being a class or a namespace. I saw this namespace+enum construct on StackOverflow. It allows to use Level::Type as a type, and Level::H1 for the elements. > @@ +152,3 @@ > + } > + //NOTE: Gtk::Menu is a Gtk::Container which has a foreach function > + //void Gtk::Container::foreach(const ForeachSlot& slot) > > What is the point of this comment? it's just hacking notes, not to be included in the final patch. Tomboy/C# uses a foreach here, that seemed more clear to me, but I did not know how to use the ForeachSlot& yet. I removed the comment. Thank you for all the comments! See the updated patch
Created attachment 241687 [details] [review] gnote-3.6.0_tableofcontent-0.6.patch
Created attachment 241764 [details] [review] Add-in implementation I've ported add-in to git master version of Gnote. I haven't drilled it much, just a couple things to note: 1. I think it's worth to add gui to toggle headers on/off, I think TOC menus are suitable places for this 2. I don't like this huge about text, that is shown in menu, with (1) implemented we can get rid of it 3. Manual page is in TODO, it should contribute to resolving (2). Note, that we use Mallard format now for documentation.
(In reply to comment #14) > Created an attachment (id=241764) [details] [review] > Add-in implementation > > I've ported add-in to git master version of Gnote. Ah great! > I haven't drilled it much, just a couple things to note: > 1. I think it's worth to add gui to toggle headers on/off, I think TOC menus > are suitable places for this > 2. I don't like this huge about text, that is shown in menu, with (1) > implemented we can get rid of it > 3. Manual page is in TODO, it should contribute to resolving (2). Note, that we > use Mallard format now for documentation. Yes I agree with you. It's not very conventional. It was part of further plans (http://is.gd/gJEBdV). It could look like this, tell me what you think: | | | Table of Content Ctrl-Alt-1 > |+----------------------------+ +-----------------------------------+| (empty table of content) | |----------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | +----------------------------+ With GTK# I failed to get the accelerator (Ctrl-Alt-1) on a menu item when there is a submenu. So it could be also like this: | | | Table of Content > |+----------------------------+ +-----------------------------------+| (empty table of content) | |----------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | | Show popup Ctrl-Alt-1 | +----------------------------+ There could be also a direct access to the help page |----------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | | Show popup Ctrl-Alt-1 | | Help | +----------------------------+ We could keep the note title in any case, this would allow to "jump to top" on any note. | | | Table of Content > |+----------------------------+ +-----------------------------------+|[] *Note title* | | (empty table of content) | |----------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | | Show popup Ctrl-Alt-1 | | Help | +----------------------------+
Review of attachment 241764 [details] [review]: ::: src/addins/tableofcontent/tableofcontent.desktop.in @@ +8,3 @@ +DefaultEnabled=false +Module=tableofcontent +_Copyright=\xc2\xa9 2013 Luc Pionchon maybe _Authors and _Copyright should not be translatable
Created attachment 241840 [details] [review] gnote-3.9.0_tableofcontent-0.7.patch Here comes an update with - help page - menu entries for commands, with accelerators - when toc is empty, no more long help text | | | Table of Content > |+----------------------------------+ +------------------------+| (empty table of content) | |-----------------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | | Show in a Popup Menu Ctrl-Alt-1 | +-----------------------------------+ It starts to smell like almost cooked. NOTE: I used the GNOME 3.6 platform, so I downgraded the library requirements [1] to compile the master/HEAD tree. It seems to just work. Is the lib upgrade really needed? [1] https://git.gnome.org/browse/gnote/commit/?id=8e2f4ca4aab3f356825b9846d03f8119d85709ac
(In reply to comment #14) > Created an attachment (id=241764) [details] [review] > Add-in implementation this is now obsoleted by latest update. But for some reason bugzilla disallows me to edit it or set it obsolete
(In reply to comment #17) > | | > | Table of Content > |+----------------------------------+ > +------------------------+| (empty table of content) | > |-----------------------------------| > | Level 1 header Ctrl-1 | > | Level 2 header Ctrl-2 | > | Show in a Popup Menu Ctrl-Alt-1 | > +-----------------------------------+ I suggest don't think we need this popup item. There's no real use of it except for info, but that's what manual is for. I also think there's no need for "(empty table of content)". Let's just have the two header items and when there is the real TOC, have it above those two items. That way we'd have a cleaner and simpler UI and the same sunctionality. What do you think? > It starts to smell like almost cooked. Yes, I think of including it in the nearest release. I'll have thorow review when time permits and push it in. GREAT JOB! > NOTE: I used the GNOME 3.6 platform, so I downgraded the library requirements > [1] to compile the master/HEAD tree. It seems to just work. Is the lib upgrade > really needed? GtkSearchEntry is only supported ince GTK 3.6. You said you're using 3.5, which is a development version, so it might be in there (since some point release).
(In reply to comment #19) > (In reply to comment #17) > > | | > > | Table of Content > |+----------------------------------+ > > +------------------------+| (empty table of content) | > > |-----------------------------------| > > | Level 1 header Ctrl-1 | > > | Level 2 header Ctrl-2 | > > | Show in a Popup Menu Ctrl-Alt-1 | > > +-----------------------------------+ > > I suggest don't think we need this popup item. There's no real use of it except > for info, but that's what manual is for. I tend to agree though I am thinking that if it is not in the UI, it's like a hidden feature. Shortcuts are for advanced users (right?) who might expect to have all the information in the UI, without need to read a manual. On another hand the fact that it is not sensitive can be confusing, like if the feature was never available - so I think it should go away. What I think we should have is: | Table of Content Ctrl-Alt-1 > | +------------------------------- + but I did not manage to get it so far, any hint? > I also think there's no need for "(empty table of content)". Let's just have Actually I think that it has some use. It shows that the toc will come here when there will be headers. It shows that the menu is dynamical. The "What links here?" plugin has a "(none)" element when empty, in the same way. Note that it is not sensitive, so greyed out = quite clean. Also if you imagine the toc menu with just the "Level x Header" entries, it might be very confusing to understand, because "Level x Header" does nothing visible, unless you selected some text first. I could imagine a first time user clicking on "Header 1 Level" and nothing happen, and later when he starts to type again on the note, the text is bold+huge, very confusing. > the two header items and when there is the real TOC, have it above those two > items. That way we'd have a cleaner and simpler UI and the same sunctionality. > What do you think? > > > > It starts to smell like almost cooked. > > Yes, I think of including it in the nearest release. I'll have thorow review > when time permits and push it in. > GREAT JOB! Thank you! > > > NOTE: I used the GNOME 3.6 platform, so I downgraded the library requirements > > [1] to compile the master/HEAD tree. It seems to just work. Is the lib upgrade > > really needed? > > GtkSearchEntry is only supported ince GTK 3.6. You said you're using 3.5, which > is a development version, so it might be in there (since some point release). Ah, yes you are right, I have 3.5!
Created attachment 241860 [details] [review] gnote-3.9.0_tableofcontent-0.7_help-entry.patch This is a small patch to add a menu entry to open the toc addin help page from the toc menu. To apply on top of gnote-3.9.0_tableofcontent-0.7.patch There are pros and cons, IMHO What do you think?
> was never available - so I think it should go away. What I think we should have > is: > > | Table of Content Ctrl-Alt-1 > | > +------------------------------- + > > but I did not manage to get it so far, any hint? You can create MenuItem passing a Widget as an argument to constructor. That Widget could be Gtk::Box with two labels. > > I also think there's no need for "(empty table of content)". Let's just have > > Actually I think that it has some use. It shows that the toc will come here > when there will be headers. It shows that the menu is dynamical. The "What > links here?" plugin has a "(none)" element when empty, in the same way. Note > that it is not sensitive, so greyed out = quite clean. Well, backlinks have such item, because otherwise it would have an empty meniu. In this case menu is not empty and it's dynamic nature becomes evident once you open this menu after turning on a header. With help item added I think it's quite enough. > Also if you imagine the toc menu with just the "Level x Header" entries, it > might be very confusing to understand, because "Level x Header" does nothing > visible, unless you selected some text first. I could imagine a first time user > clicking on "Header 1 Level" and nothing happen, and later when he starts to > type again on the note, the text is bold+huge, very confusing. I tend to put everyday user need a priority over first time users. Besides this is no more or less confusing than standard items like bold, italic etc.
(In reply to comment #21) > This is a small patch to add a menu entry to open the toc addin help page from > the toc menu. [...] > What do you think? I think help should go in and that dummy "no TOC" out. Unless you have additional arguments on why we should make it otherwise. Just a couple things to note on help item: - remove italic markup, make it regular text - put a separator between this item and the rest - make text something like "TOC help" instead of just "Help"
(In reply to comment #22) > > > > | Table of Content Ctrl-Alt-1 > | > > +------------------------------- + > > > > but I did not manage to get it so far, any hint? > > You can create MenuItem passing a Widget as an argument to constructor. That > Widget could be Gtk::Box with two labels. it did not work very well: MenuItem in a menu has three areas: [img] + widgets (usually a label) + accelerator. By using a box and two labels, the 'widgets' area will request space for both labels. This even though we align the second label to the right It results a large gap between labels and accelerators. But after some struggle I finally managed to get it correctly (I think): Looking at MenuItem source code helped: MenuItem is using an AccelLabel. So it is just about setting the accelerator on the AccelLabel. For this we need a recent function, which I am missing on my libgtkmm, but not on my libgtk+, so I used the gtk+ function. You'll see in the updated patch: /* TO UNCOMMENT *///acclabel->set_accel ( ... /* TO DELETE */gtk_accel_label_set_accel (acclabel->gobj (), ... It just works as is, but in the final patch, it's better if we use the gtkmm function. > I tend to put everyday user need a priority over first time users. Besides this > is no more or less confusing than standard items like bold, italic etc. fair enough (In reply to comment #23) > I think help should go in and that dummy "no TOC" out. Unless you have > additional arguments on why we should make it otherwise. > > Just a couple things to note on help item: > - remove italic markup, make it regular text I have been using italic to differentiate with the toc content. > - put a separator between this item and the rest I don't fully understand. Currently it is like this +-----------------------------------+ | toc item | | toc item | |-----------------------------------| | Level 1 header Ctrl-1 | | Level 2 header Ctrl-2 | | Table of Content Help | +-----------------------------------+ Do you want a separator between Level 1/2 actions and Help ? > - make text something like "TOC help" instead of just "Help" done I have been pondering the need of the Help menu entry. Now that all accelerators are displayed, there is less need. I would tend to remove it. Independently, I think that a good place to give access to a add-ins specific help pages, would be in the preference dialog, near the information button. That's probably the place where we first learn about them, and need the help page.
Created attachment 242375 [details] [review] gnote-3.9.0_tableofcontent-0.8.patch updated patch with latest comments from Aurimas, thanks! == 0.8 - Added Help entry in toc menu - Set Ctrl-Alt-1 accelerator on toc menu item - Removed "Show in a Popup Menu" entry in toc menu - Don't show action entries in detatched popup. - Don't show (empty toc) label on menus with action entries. - Removed italic on action menu entries
Created attachment 242468 [details] [review] gnote-3.9.0_tableofcontent-0.9.patch cleanup/unifying/polishing update == 0.9 - renamed labels "Level 1 Header" to "Header Level 1" (resp. 2) - help topic major update - set help topic status to "candidate" - updated description in .desktop - TODO major update from my local notes, see: Todo, Maybe and Maybe not - code: general code review, various minor updates in code, including: - add-in description in copyright headers - various code comments - renamed header level enum: s/Level/Header/ and s/H1,H2,H3/Title,Level_1,Level_2/ (so forget the "HTML alike", now Ctrl-1/2 matches Level_1/2 (instead of H2/H3!)) - renamed get_range_level() into get_header_level_for_range() Now it should be pretty much as we discussed. Tell me if I accidentally missed something! I still plan to do: - On Ctrl-1/2 toggle, toggle only B/L/H tags, not all tags (this is a bug) - Check GFDL usage in help topic And maybe these could be nice: - Huge is not huge enough, see https://bugzilla.gnome.org/show_bug.cgi?id=657601 - Highlight the header when jumping to it
I've pushed this in. Looks good enough. Please, continue the great work and open new tickes with patches. Thanks.
Oh great! I am very happy to see this in. So it should now enter the translation loop, and should show up in Linux distributions at the time of GNOME 3.10. This is great news. More people will be able to access it. I hope that it will be useful to many of them. Thank You and Hubert for the nice comments, the code reviews and hints, and to take it in the upstream tree! Thank you, Luc
Btw, what file(s) and instructions should I provide to volunteers for testing/trying it ? With Tomboy and C#, it is just about the TableOfContent.dll file and dropping it in the user Tomboy/config folder, it will work for many Tomboy releases. How is it working with Gnote/C++ ?
ATM it would require user to have most recent Gnote from git master, as add-in support have been changed. To use add-in you need to put tableofcontent.so and tableofcontent.desktop to ~/.config/gnote/addins directory.
Created attachment 242749 [details] tableofcontent-3.6.0-0.10pre.so I backported the addin to gnote 3.6.0, so people can try it and send feedback. Hopefully it works also with gnote 3.8 (?)
Created attachment 242750 [details] tableofcontent-0.9.page This is the matching help page. It can be viewed by running: $ yelp tableofcontent-0.9.page (in the Table of Content menu, the help entry will fail because the help page is not installed) As Aurimax told above, the addin file tableofcontent-3.6.0-0.10pre.so must be dropped in: ~/.config/gnote/addins/
Created attachment 243043 [details] tableofcontent-0.8.4-0.9.so the add-in recompiled in a gnote-0.8.4 tree.
Created attachment 243278 [details] gnote-0.8.4--toc--no-locale.tgz source tarball for gnote-0.8.4 + tableofcontent add-in. (minus the locales to make the tarball smaller)