We should be able to get improvements in both function and performance by integrating with the new menu system.

See:
Core hierarchical page structuring module
http://groups.drupal.org/drupal-dojo/book-project

These improvements require this patch to go in first: http://drupal.org/node/145058

CommentFileSizeAuthor
#197 better_book_69.patch91.66 KBpwolanin
#196 better_book_68.patch90.66 KBpwolanin
#187 better_book_67.patch89.08 KBpwolanin
#185 better_book_66.patch88.81 KBpwolanin
#184 better_book_65.patch88.1 KBpwolanin
#182 better_book_64.patch88.01 KBpwolanin
#181 better_book_63.patch86.46 KBpwolanin
#176 better_book_62.patch87.8 KBpwolanin
#173 better_book_61.patch86.08 KBpwolanin
#172 better_book_60.patch84.46 KBpwolanin
#170 better_book_59.patch81.94 KBpwolanin
#162 better_book_58.patch79.7 KBpwolanin
#159 better_book_57.patch78.5 KBpwolanin
#156 better_book_56.patch78.66 KBpwolanin
#154 better_book_55.patch78.6 KBpwolanin
#149 better_book_54.patch75.71 KBpwolanin
#146 better_book_53.patch75.71 KBpwolanin
#140 better_book_52.patch75.69 KBpwolanin
#136 better_book_51.patch75.92 KBFrando
#132 better_book_50.patch75.82 KBFrando
#127 better_book_49.patch75.79 KBpwolanin
#122 better_book_48.patch71.99 KBpwolanin
#117 book_to_outline_47.patch72.92 KBpwolanin
#112 book_to_outline_46.patch73.03 KBpwolanin
#110 book_to_outline_45.patch72.98 KBpwolanin
#107 book_to_outline_43.patch72.93 KBpwolanin
#106 book_to_outline_42.patch71.32 KBpwolanin
#105 book_to_outline_41.patch69.87 KBpwolanin
#95 book_to_outline_0.patch67.68 KBhunmonk
#94 book_to_outline.patch67.69 KBhunmonk
#93 book_to_outline_38.patch68.54 KBpwolanin
#91 book_to_outline_37.patch68.12 KBpwolanin
#90 book_to_outline_36.patch68.21 KBpwolanin
#86 generate_book6.zip_.doc3.01 KBpwolanin
#80 book_to_outline_35.patch68.19 KBpwolanin
#79 book_to_outliner_18.patch68.14 KBdmitrig01
#78 book_to_outliner_5.patch68.05 KBdmitrig01
#77 book_to_outliner_3.patch67.34 KBdmitrig01
#76 book_to_outliner_0.patch67.36 KBdmitrig01
#75 book_to_outline_34.patch66.94 KBpwolanin
#73 book_to_outline_33.patch67.17 KBpwolanin
#72 book_to_outline_32.patch66.52 KBpwolanin
#71 book_to_outline_30.patch66.41 KBpwolanin
#67 book_to_outline_29_0.patch64.89 KBpwolanin
#64 book_to_outline_29.patch66.93 KBpwolanin
#59 book_to_outline_28.patch73.08 KBpwolanin
#57 book_to_outline_27.patch66.64 KBpwolanin
#54 book_to_outline_26.patch66.43 KBpwolanin
#47 book_to_outline_25.patch66.49 KBpwolanin
#44 book_to_outline_24.patch67.04 KBpwolanin
#42 book_to_outline_23.patch66.97 KBpwolanin
#32 book_to_outline_22.patch66.9 KBpwolanin
#31 book_to_outline_21.patch65.09 KBpwolanin
#29 book_to_outline_20.patch64.25 KBpwolanin
#23 book_to_outliner_17.patch58.72 KBpwolanin
#22 book_outliner_1.patch59.05 KBdmitrig01
#21 book_outliner_0.patch58.99 KBdmitrig01
#20 book_outliner.patch59.44 KBdmitrig01
#19 book_to_outliner_16.patch57.3 KBpwolanin
#18 generate-50book.php_.txt5.09 KBpwolanin
#16 book_to_outliner_15.patch53.14 KBpwolanin
#15 book_to_outliner_14.patch52.11 KBpwolanin
#14 book_to_outliner_13.patch50.85 KBpwolanin
#13 book_to_outliner_12.patch48.89 KBpwolanin
#12 book_to_outliner.patch42.19 KBdmitrig01
#11 book_to_outliner_11.patch42.09 KBpwolanin
#10 book_to_outliner_10.patch42.1 KBpwolanin
#9 book_to_outliner_9.patch40.44 KBpwolanin
#8 book_to_outliner_8.patch40.23 KBpwolanin
#7 book_to_outliner_7.patch38.74 KBpwolanin
#6 book_to_outliner_6.patch34.32 KBpwolanin
#5 book_to_outliner_4.patch30.72 KBpwolanin
#3 book_to_outliner_2.patch20.91 KBpwolanin
#2 book_to_outliner_1.patch1.87 KBpwolanin
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pwolanin’s picture

pwolanin’s picture

Status: Active » Needs work
FileSize
1.87 KB

initial schema patch:

mlid - will allow join to menu_links
nid - for nodes
bookid - will equal the nid of the top-level node
child_type - define the node type for "add child page" link

pwolanin’s picture

FileSize
20.91 KB

starting to hack at the code- nothing functional yet.

moshe weitzman’s picture

subscribe

pwolanin’s picture

FileSize
30.72 KB

snapshot of progress.

pwolanin’s picture

FileSize
34.32 KB

another snapshot

pwolanin’s picture

FileSize
38.74 KB

module installs, functions starting to work (a little)

pwolanin’s picture

FileSize
40.23 KB

more functionality

pwolanin’s picture

FileSize
40.44 KB

book export working reasonably

pwolanin’s picture

FileSize
42.1 KB

better yet

pwolanin’s picture

FileSize
42.09 KB

mostly just CSS and admin pages need fixing.

dmitrig01’s picture

FileSize
42.19 KB

here is a slight schema change... bookid => book_id in indexes

pwolanin’s picture

FileSize
48.89 KB

more working- changed menu callbacks so enable or diable a module to trigger a menu rebuild

pwolanin’s picture

Status: Needs work » Needs review
FileSize
50.85 KB

Admin pages seem to work now.

pwolanin’s picture

FileSize
52.11 KB

fixed a bug with removing pages form the outlien

Also, modified .info file and code in hook_uninstall

pwolanin’s picture

FileSize
53.14 KB

fixed the problem with book navigation - the calls to l() had not been converted to use the new $options array and so weren't getting the right CSS classes applied.

Also, put the right value in menu_links for 'module'. Simplify uninstall code. Re-roll for new FAPI 3 parameter order for hook_form_alter. Fixed bugs in hook_nodeapi, and book_outline_submit.

Except for the update path I think this is very close to RTBC- review, please!

drewish’s picture

subscribing

pwolanin’s picture

FileSize
5.09 KB

book node generation script for 5.x to test update. Adapted for a 4.7 Devel module script.

Put at root level in your Drupal directory and visit the script. Edit the code to change the # of nodes generated.

You should probably generate a few users by hand or with Devel module first.

pwolanin’s picture

FileSize
57.3 KB

now with update code!

dmitrig01’s picture

FileSize
59.44 KB

first try @ an upgrade path

dmitrig01’s picture

FileSize
58.99 KB

update.php stuff got in...

dmitrig01’s picture

FileSize
59.05 KB

need node_types_rebuild and menu_rebuild after node type save

pwolanin’s picture

FileSize
58.72 KB

re-rolled with based on code comments and suggestions on IRC from dmitrig01 .

fixed book_node_visitor_html_pre() missing parameter. RTBC?

morphir’s picture

Status: Needs review » Reviewed & tested by the community

tested. Seems to work flawlessly.

This baby is ready to fly. RTBC

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This patch is poorly reviewed -- doesn't buy you guys much reviewer points. :/

1. Total lack of code comments in book_form_alter() and book_outline() -- just to name two functions.

2. Coding style issues. For example, in _book_add_form_elemts() we are using tabs.

3. Inconsistent capitalization and punctuation in PHPdoc.

4. I'd prefer to get rid of some of the book terminology. People new to Drupal want to structure pages, not create books. If this is too much work, we can leave that for another patch.

5. I don't quite understand what the $child_type is for?

6. What I don't like about this patch is that the book.module now accesses all of the menu system's internals. It shows that the menu system's API are far from perfect.

7. I'd like to get rid of things like 'Move to another book'. This shouldn't be an explicit operation, IMO. It should be a matter of selecting a new parent in the drop down menu. This patch suffers from feature creep. It introduces the notion of separate books. Please don't. That should go in a separate patch. (We want to remove the notion of a book, not make it more book-y.)

8. I was confused by the name 'book_link'. It doesn't seem to contain what I expected it to contain. What exactly is it for?

9. Shouldn't we be able to remove the mass book administration page? That should probably be part of the menu module now?

pwolanin’s picture

@Dries - I will address code style and comment issues as soon as I can (tonight).

I think the module renaming should wait for another patch, but I will very happily take that on once this gets in.

Part of the point of the way this is coded is to avoid any dependence on menu module. Menu module is now an optional module and, in a sense, this becomes an alternative to menu module. So, I think the fair comparison in terms of use of menu.inc functions is menu module itself.

Also, menu module shouldn't operate except on menu links that and menus that it created itself. The book administration page is a convenience feature, but something menu module can't offer.

$node->book_link contains a menu link, with additional book-module data.

$node->child_type determines the type in the "Add child page" link. Since a hard-coded 'book' type is gone, we need some way to specifiy this type. Having this also lets different books have different child-type links, since this property is automatically inherited from the parent node for non-admins.

I think the separate book notion is a critical feature - this makes the drop-down size manageable, and enables a lot of other possibilities. I thought this was the time for new features? Obviously, adding more JS prettiness to this UI later is an important possibility.

Frando’s picture

Hey, this is looking great. I'll do a more extensive review of your patch soon.

1. It would be great if it would be possible to not only have nodes in a book. One might want to have a view or a panel be part of an outline as well. It would be great if e.g. Views could just somehow add a form defined by book.module to its UI that allows a view to be inserted into a book.

2. Concerning seperate books: This is not required as long as we can easily get the top-level links. IMO, there is no difference between a top-level link and its children and seperate books, but the latter is probably a little more confusing. Concerning the UI, something like two drop-downs would be great, the first listing all top-level links, and the second one the children of the first (active select):

Place in outline [DROPDOWN: before | after | as child of]  [DROPDOWN: top level items, "books"] [DROPDOWN: children | of | selected | book]
pwolanin’s picture

@Frando - being able to add non-node pages into books is something this patch moves closer to enabling. However, there are some difficult issues with making that work smoothly that I don't want to address here.

Also, I don't really understnad your suggestion re: 2 drop down. I think if we can AJAX the form, we can make this UI better (e.g. select the book, then the second drop-down or other interface is filled in with appropriate links), but it's not going to change the underlying functionality that's in place in this patch.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
64.25 KB

massively updated code comments.

greggles’s picture

Regarding:

7. I'd like to get rid of things like 'Move to another book'.

I find that a great feature of this patch. I've heard both 7 and 12 used as upper limits on the number of items to display in a list before that list should be broken down into smaller pieces. In my use of the book module, the current parent dropdown frequently has hundreds or even thousands of items in it. By splitting the "move to a new book" functionality into a new area the dropdown is much smaller which is a great improvement in usability in my opinion. Just today Ezra and Laura were complaining about the hundreds of items in a book on a knowledgbase site and talking about using form_alter and other custom code to achieve what this patch provides: simplification of the parent dropdown.

One alternative would be to change the UI to use the lumen menu system which allows quick drill down into deeply nested lists.

pwolanin’s picture

FileSize
65.09 KB

fix book TOC in the book navigation

pwolanin’s picture

FileSize
66.9 KB

after several discussions on IRC:
-I added an AJAX callback (though Josh K will comment on whether or not this is necessary).
-To help enable an add-on JS UI, I also put the form in the Outline tab in the same fieldset as on the node form.
- added a 'menu_name' attribute to the select for use with the AJAX callback.
-Per request of Josh K and others, added to the node type form a setting to exclude a node type from being added to book outlines.

Dries’s picture

pwolondin: I think we're trying to do to much in this patch.

Things I'd like to see removed:

1. Move to different book. This should go into a different patch.

2. Child page type. I really do not understand what this is for. The UI is confusion and doesn't give me a clue as why this is useful. Care to elaborate?

What I'd like us to see focus on instead is:

1. Proper integration with the menu system.

pwolanin’s picture

@Dries - I can try to clarify child_type in the form text. Simply put - all operations are handled by hook_nodeapi after this patch. So, there is no default "book" type and we need a way to specify the type that appears in the "Add child page" link. Doing this per node adds a lot of flexibility. Also, this option only appears for users with 'administer nodes' permission, so it won't be a concern for non-admins. For normal users, it inherits from the parent node.

@Dries - The menu system integration means that there is a 1-1 mapping between a "book" and a "menu". So this is the most logical way to code the integration. Changing the way this is operating in book would require substantial re-coding, and I think it is an important feature in terms of usability, reducing the amount of data loaded, etc. In any case, I expect we will continue to try to improve the UI with subsequent patches.

Dries’s picture

Can't you do a generic "add child" that lets me chose from different node types? I don't see why we shouldn't be able to mix different child types easily.

pwolanin’s picture

@Dries - the reason I like this is that it means that different books can be set to have different child types. For example, if sepeck splits the handbooks into 'official' and 'unofficial', the 'offical' books could have a child-type node that can only be added by a member of the docs team, while 'unofficial' pages have a child types that can be added by any user.

Any type can be added as a child to any node via the outline tab for a user with the 'Outline posts in books' permission. I think this is an important granularity in permissions, and is a closer mimic to the current functionality.

moshe weitzman’s picture

'add child' is poor language compared to 'add product' or 'add policy' (i.e. custom types). furthermore, an 'add child' link would send you to an intermediate page where you choose from available content types and then you would start authoring. bleh. my .02.

pwolanin’s picture

@moshe - customizing the link is easy - becomes "Add story as child" or "Add new story"? I think having an intermediate form would be aggravating, plus see above.

$info = node_get_types('type', $node->child_type);
t('Add @type_name as child', array(@type_name => $info->name));
pwolanin’s picture

@mosh, Dries - how about an additional link and intermediate form that are only available to users with the 'Outline' permission (note, however, I can't do any coding til tonight - and not much even then). Or, this can be an additional patch if it's not very important to get done now?

Dries’s picture

I think we need to leave all of these new features out or we risk missing the code freeze. I already mentioned what features I'd like to see removed from this patch.

joshk’s picture

I think it Dries says cut back on features we should cut back on features. This patch is becoming somewhat huge.

As for the AJAX callback, it is unnecessary IMHO. The way the list of OPTIONS is set up for the select element, it will be totally possible to use jquery to extract a useful data structure and implement an interactive parent-picker. I don't have working JS for this (yet), but this loop lets me extract the options in order and establish their parentage:

$.each($('#edit-book-link-plid-wrapper option'), function(index, option) {
  console.log($(option).html());
});;

If we go back to the old-style UI of having all the options in one select, that would be easiest to enhance with JS I believe.

pwolanin’s picture

FileSize
66.97 KB

@josh_k : The only thing easy to cut is the AJAX callback I just added.

I'm frustrated. I want to make this module *better*, and I tried to follow this formula:

Remove the "book page" node type, and turn the book module into a module that is specialized at creating hierarchical content trees. The module should automatically extract a menu from the node hierarchy, set the correct breadcrumbs, help you define a hierarchical URL structure with clean URLs, etc. This module will be the de facto standard to structure pages and to create hierarchical navigation schemes in Drupal.

we're not quite there yet (e.g. no clean URL integration yet), but closer than before.

Patch attached- removes unneeded AJAX callback. Also includes a fix for the query in menu_tree_all_data(), which the menu module patch must have broken.

pwolanin’s picture

I see know, the broken query was probably from this bad commit: http://drupal.org/node/147873

however, the commit seems not to have been fully rolled back.

pwolanin’s picture

FileSize
67.04 KB

fixes notice:

notice: Undefined variable: output in /Users/Shared/www/drupal6/modules/book/book.module on line 927.

Also, additional whitespace and comment cleanup.

pwolanin’s picture

need to rewrite the update function per: http://drupal.org/node/144765#comment-245805

Dries’s picture

No need to be frustrated. I disagree with some of the extensions you made, not with the overall direction of this patch. We both want to convert the book module into a powerful outliner that allows you to created structured pages. We share the same goals. :)

You're making it more book-like, ultimately, I want to make it less book-like. Making it less book-like is key if we want this to be _the_ outliner feature. We have to stop using the wrong terminology, and as such, the notion of there being multiple books need to go. We also need to rethink the workflow.

I'd like to do a straight-forward conversion and then focus on how we can add more functionality, in such a way that it is easy and intuitive to use. For example, the proposed 'child_type' feature is currently difficult to understand and use, and therefore a step backward for many users.

pwolanin’s picture

FileSize
66.49 KB

@Dries - ok -I'm glad we're on the same page. I would just like to see this in, as the basis for making further changes. I think some concept of separate structures/outlines/etc is helpful, however, in orgnizaing a large site.

fixed (and tested) updated code in attached patch.

pwolanin’s picture

@Dries - I really don't quite understand where our points of difference are, other than relatively trivial ones of user interface. It seems to me that any module for outlining/structuring a site needs to support multiple containers/hierarchies/outlines (whatever you want to call them). In 5.x, they are only weakly separated as different "books". I think it represents an essential strengthening of the module to move to the point where these "containers" can really be treated (themed, permissioned, localized, etc) as separate entities. My thinking is informed by all the possibilities that are implemented for Organic Groups.

Are you thinking that these "containers" should not be represented by nodes? Are you envisioning something totally different?

Crell’s picture

I have not had time to track this patch and have not looked at the implementation, so take my comment with the appropriate salt lick. :-) However, I have to agree conceptually with pwolanin. Whether an "outliner" is being used as a book or not, multiple separate "hierarchy things" is an important feature. Drupal.org may only need one hierarchy that it can use for all handbooks, but there are absolutely use cases where you need them separate. For instance, put OGs into a hierarchy and have a separate collection of multi-page handbooks on the same site that are independent of them (which I would use this for). Forcing them to all be one structure is just as limiting as saying that the Navigation Menu (mid 1) is all a site really needs.

dww’s picture

i think the only difference between pwolain and dries on this is the terminology, both in the UI and in the code. i (hope) we're all in agreement at separate top-level outlines/containers/books/whatever is a good move. see http://drupal.org/node/81226 for some prior effort/thought on that topic.

also, d.o better not just setup a single "handbooks" outline, or the UI benefits aren't going to be much. we should have separate outlines for each "book" in the handbooks now. then, the "move to a new outline" will make life *much* easier for editing, expanding, and maintaining the d.o handbooks.

if i didn't have too many of my own battles to wage right now, i'd tear into this issue more closely and help get it into D6. but, it seems to be in good hands, so i'll focus my efforts elsewhere...

dww’s picture

whoops, actually http://drupal.org/node/65319 was the issue i was talking about, but that ended up getting re-started as http://drupal.org/node/81226. ;) so, both are worth reading for the interested historian...

sepeck’s picture

I have always wanted separate outlines for each book (preferably still be able to move content between books). I have additional hopes for separate permissions for each book as well and can produce several use cases if needed.

catch’s picture

/also hasn't had a proper look at the patch so big chunks of salt/

I have some full length books on my site, organised in chapters, quite a lot of them. To me that's book working as essentially a paged article with extra menu and navigation links. But we also have a multi-level quasi-handbook organised with book.module. I've also been following discussions about using the outliner + views to replace the forum index if you were able to add views (and panels) to outlines - which may be an option later on from either end (panels2). Seperate outlines would be a god send to manage that sort of thing.
Also with child types, it'd help a lot to seperate 1920's political texts from HOWTOs, and avoid using "book parent" filters all over the place.

pwolanin’s picture

FileSize
66.43 KB

per this devel list discussion: http://lists.drupal.org/pipermail/development/2007-May/024194.html and this issue: http://drupal.org/node/148420 I changed the code so all book module data is held in the array $node->book

Also, fixed a few PHP notices, and added a message to the node_delete_confirm form.

dww’s picture

Status: Needs review » Needs work

@pwolanin: cool, thanks for the new patch that follows one possible solution to the $node namespace stuff over at http://drupal.org/node/148420. However, I fear it's premature, since we haven't converged on a solution yet (module-based, schema-based, hybrid, etc).

Also, I believe Dries wants this to not retain the name "book" at all. From what I gather from his comments here and in other places, he wants this to be the "outline" module, not the "book" module -- in name, in UI, in the code, and in the $node object. ;) Setting this to needs work since I think a complete renaming is going to be required before this lands. Dries, please correct me if I'm wrong...

pwolanin’s picture

@dww - regardless of where http://drupal.org/node/148420 ens up, clearly we're moving towards (at the least a best practice) of keeping things out of the top-level of $node.

Since we seem to have a little time, I can do a rename if Dries is will to give me a specification ASAP. The only tricky part about that- how do we handle the update? Put a check in the X_install() function to see if {book} exists or if book module is in the {system table}? Should it delete book from the {system} table?

pwolanin’s picture

FileSize
66.64 KB

per http://drupal.org/node/147873 change the menu query to be Oracle-friendly.

pwolanin’s picture

@Dries - should the module be renamed in this patch?

In thinking about this, 'child_type' could equally well be set per node or per "container". Are there settings that should be supported in core on a per-container (a.k.a. per-book) basis?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
73.08 KB

new patch to accommodate FAPI changes. Setting to "needs review" as a synonym for "needs feedback"!

Dries’s picture

I spent another hour with this patch.

When I create a page (like an about page), I expect that I can add it to the navigation menu. I want the book module to integrate with the page node type so I can start building structural pages right from the start. To make that work, the book module needs to integrate with the "create page" form. Use case: create an about page, with a 'staff pages' below that. The about page is of type 'page', the pages of type 'employee' (CCK node type). Thus, the book module works like a very simple view. Either way, I want us to optimize this module for this use case. There should be no special configuration steps required -- the learning curve has to be flat.

Specifically:

  1. I think we should enable this module by default. Please add it to the default install profile.
  2. The outline-tab must become a fieldset on the node edit page so that I can outline the page upon creation time. Creating a child page should be done in one step, not two steps. Remember we're trying to lower the barrier.
  3. Specifically, I'd look at how this can be integrated with the menu module. The menu module already asks you to provide a "parent" and a "weight". The book module ask you the exact same information. Integrating the book and menu module will lower the barrier even more.
  4. The "child page type" feature needs to be removed. One should be able to add _any_ node as a child.
pwolanin’s picture

@Dries - there is a problem with what you are asking for, which mainly pertains to the book navigation elements and navigation through the hierarchy.

If the book hierarchy can include non-node pages (implicit if we're adding to Navigation), then many of the fundamental assumptions of the module break. For example, Drupal has no easy way to have me show "Add child page" on /admin.

Or, is what you're saying that each book should get added to Navigation, rather than being listed separately at /book?

pwolanin’s picture

@Dries - reading again what you wrote, it seems like you are asking for what the menu module already does with menu-on-the-fly in the node form. I think the menu module should be enabled by default, and we should consider relocating the menu module's fieldset on the node form so it's more visible.

pwolanin’s picture

@Dries - this thread (though a couple years old) may be worth looking back at: http://drupal.org/node/31896

I think we need to distinguish among use cases:

1) brochure/static site
content added mostly or entirely by a few admins, who may be novices
relatively small (~10-100 pages)
all links can be in the navigation menu or primary links
links are a mix of node and callbacks

2) informational site
content added mostly or entirely by a few admins
relatively large (~100's-1000's of pages)
need a separate menu for each site section (e.g. FAQ vs. user guide)
links are nodes

3) community site
content added by many users (non-admins)
relatively large (~100's-1000's of pages)
need a separate menu for each site section (e.g. FAQ vs. user guide)
may need different access control for different site sections
links are nodes

The thread I reference above deals essential with case #1. I think menu otf is really the right solution for this case, so we should think about how to make it most accessible/usable for new admins. This also seems to be use case you have in mind.

For book module, I have in mind use cases #2 and #3. #3 can be represented by the d.o handbooks, and is the use case *I* have in mind as I'm trying to improve the book module. Even though book module may also be useful for case #1, I think it would be a huge mistake to orient the module to that use case.

pwolanin’s picture

FileSize
66.93 KB

re-roll for current HEAD - also previous patch picked up some unrelated changes to other files.

joshk’s picture

Suggestions from a discussion w/pwolanin in IRC:

1) Drop the child page setting. It seems obscure. A better default is to assume the node-type of the parent is the default for "add child page" links.

2) Implement hook_perm so that each "book" (that is, each root) has two permissions: add child pages, and edit outlines. This will determine whether a user gets the add child page link, and whether or not they can move nodes around, into or out of the outline from some other interface.

3) In terms of putting an outline fieldset on the node/add and node/edit interface, there's an issue of what to do for sites that have huge numbers of book pages. The arbitrary yardstick is drupal's ~10,000 pages. One school of thought is to have a 2-part interaction for moving nodes between books (similar to upload.module's handling of file-uploads), another is that we have to solve the 10,000 node problem sooner or later anyway.

joshk’s picture

More on permissions:

If we assume that child-pages are the same (by default) as the root node, and we implement a per-book permission to add them, there could be a conflict where a user role has the "add child page" permission for a book, but not the necessary access to create pages of the default child type.

I think this is a possible configuration issue that we may want to alert admins of, but I still thing having explicit per-book permissions for adding child pager and editing outlines is the way to go.

Also, I think it makes sense to assume that any role with "administer nodes" can add child pages and edit outlines for any book.

pwolanin’s picture

FileSize
64.89 KB

following up on discussion with Josh- this is a partial recode that makes the child-type link fixed as the type of the top node in the book.

I think it all works, but I didn't test the update code yet.

pwolanin’s picture

After sleeping on this, I still think that using node types are the basis for whether one can add posts to a book.

First off, this will provide a fairly transparent upgrade from 5.x - users that could previously add nodes of type "book" will still be able to add to the book outline.

Second, I think that having 2 permissions per book will become very unwieldy on the Access control page- 12 books == 24 permissions. Making or replicating node types is pretty easy, and since typically one node type can be used for several books, the net number of permissions will usually be less. The only case my suggestion degrades in terms of the total number of permissions is if you want a separate node type for each of many books. Obviously, this type-based permission will be overridden by admins with 'outline posts' or 'administer nodes' permissions.

If we can use the permission to add a node of a given type as implicit permission to add posts to a book (i.e. container) whose root node is of the same type, and if we can have either an "update" button or AJAX on the node form (in addition to what's already on the outline form) to load the right book tree, I think I can see a path forward that will make most everyone happy.

pwolanin’s picture

Also - I plan to put the module on the Drupal diet - admin and export (printer-friendly) functions will get split into a .inc file (they use in common a large function, but aren't used on normal page views)

Crell’s picture

I wouldn't worry about dieting for now. That's post-freeze performance tuning that we can do in early July. For now just worry about making it work and getting it committed. We can deal with code/file factoring later.

pwolanin’s picture

FileSize
66.41 KB

minor change to block output to avoid one link hanging out one the left.

thinking about how to put book selection in the node form, but I think I need to refactor the various form element code to avoid excess duplication.

pwolanin’s picture

Status: Needs review » Needs work
FileSize
66.52 KB

form elements are refactored so that they are common to the node form and outline tab, but the submit functions need to be re-written.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
67.17 KB

totally updated form and submit functions. Node-type permissioning. Prepared AJAX callback function per http://drupal.org/node/150859

/**
 * AJAX callback to repalce the book parent select.
 *
 * @param $build_id
 *   The form's build_id.
 * @param $book_id
 *   A book_id form from among those on  the form.
 * @return
 *   Prints the replacement HTML
 */
function book_form_update($build_id, $book_id) {

  $cid = 'form_'. $build_id;
  $cache = cache_get($cid, 'cache_form');
  if ($cache) {
    $form = $cache->data;

    // Validate the book_id.
    if (isset($form['book']['book_id']['#options'][$book_id])) {
      $book_link = $form['#node']->book;  print(drupal_to_js($form['book']['book_id']['#options']));
      $book_link['book_id'] = $book_id;
      // Get the new options and update the cache.
      $form['book']['plid'] = _book_parent_select($book_link);
      $expire = max(ini_get('session.cookie_lifetime'), 86400);
      cache_set($cid, $form, 'cache_form', $expire);
      
      $form_state = array();
      $form['#post'] = array();
      $form = form_builder($form['form_id']['#value'] , $form, $form_state);
      $output = drupal_render($form['book']['plid']);
      print(drupal_to_js($output));
    }
  }
  exit();
}

dmitrig01 promised to write the JS which should act on both the node form and the outline tab form to:

  • hide the button with ID "edit-book-pick-book"
  • when the select with ID "edit-book-book-id" changes, it should call the AJAX callback and use the output to replace the div with id="edit-book-plid-wrapper"

Needs testing!

pwolanin’s picture

ignore print(drupal_to_js($form['book']['book_id']['#options']));"in the sample code - it's not in the patch ;-)

pwolanin’s picture

FileSize
66.94 KB

minor cleanup of book.install

dmitrig01’s picture

FileSize
67.36 KB

Here is the long-awaited ajax!

dmitrig01’s picture

FileSize
67.34 KB

with the js file too

dmitrig01’s picture

FileSize
68.05 KB

for real this time

dmitrig01’s picture

FileSize
68.14 KB

fixing the base url

pwolanin’s picture

FileSize
68.19 KB

ok, after absurdly slow progress with the JS I think I have it working on both the outline tab and the node form.

Also fixed some brokeness in the submit functions.

webchick’s picture

WOW!! This patch has come an incredible way in the past little while! My only regret is I don't have a means of auto-generating 1200 bazillion pages so we can see how this would work in a handbook situation. :)

+1 for book outline options on the node form itself. The collapsed fieldset there is great, and the JS stuff for moving pages between books and such seems to work really well. I'll need to do a more thorough review of this soon (hopefully this weekend).

It sounds a bit to me though that to reconcile the conceptual differences between Peter and Dries's vision, we might need to handle renaming this module and making various UI changes in that direction at the same time. Although parts of what has been talked about leave me wondering if the "outline" feature isn't really best implemented by menu.module, and migrating legacy books to be menu paths. Maybe for D7. ;)

joemoraca’s picture

for those of us coming in very late to help test... exactly what needs to be patched in what order?
I just used a fresh install of 6 dev and the most current patch and got errors about missing fields in book table ...
I added 3 fields and still had no luck understanding how to make this come together.

pwolanin’s picture

@joemoraca: the schema is changed, so either do a clean install or run the update code.

@webchick: I think from my conversations with Dries, Josh, etc. that the menu module is the right module to be considered the default site structuring tool. See: http://drupal.org/node/151583 for a patch moving us towards making that module work better. I think (as above) the book module should remain somewhat specialized for handling large number of nodes. A fundamentally important point is, however, that book and menu will use much of the same API code, so that gives us a smaller task in terms of performance optimizations in later patches.

Note that you may want to create a additional content types to use in books, especially if you want to test the interactions with use access for user != #1.

There is a script above to generate lots of book pages for 5.x if you want to try the update code. I'll try to post one for 6.x.

dmitrig01’s picture

@webchick: re: menu links: book.module is pretty much a way to link pages to menu items, and pwolanin made a script somewhere to test it

dmitrig01’s picture

@pwolanin: please come into IRC :P

pwolanin’s picture

FileSize
3.01 KB

@webchick (and everyone) module attached - it will generate lots of content for you in the book hierarchy for testing purposes. Requires the patch on book module first, of course.

Note, rename to .zip or just unzip (extension changed to bypass upload restriction).

Dries’s picture

I had another look at this patch it is becoming rather large. I _still_ think that we need to ditch the 'Change book' functionality and just display a list of all the possible parents.

Dries’s picture

Also, this patch is becoming _huge_. I agree with the fundamentals of this patch (removing the book node type) but not with some of the details of this patch. The things we agree on can't be committed because of the things we don't agree on (yet). It's a pity. Either way, I'll try to spend some more time with this patch the next couple of weeks. :)

pwolanin’s picture

@Dries - have you tried it with the new JS helper? All possible parents are available in the form this way, without any extra clicks. Any node can now be put into a book directly from node/add as you wished, etc. I think I worked pretty hard to address the concerns you had. Note webchick's review above and the module for generating content.

Also, this is a pretty complete re-write of the module using the mneu API, so I'm not sure I understand what you mean by the patch being "large"? Trying to read the patch itself seems unlikely to be productive in this case - like with this new issue where chx just posted the new version of the comment module: http://drupal.org/node/152417

pwolanin’s picture

FileSize
68.21 KB

ah, there was a minor snafu with the JS and form code. This one should work better in picking a new parent ndoe.

pwolanin’s picture

FileSize
68.12 KB

and - forgot some debugging code

keith.smith’s picture

I applied this patch to a fresh install of HEAD today, and proceeded to beat on it with a stick for some time. I did not look at the code.

In general, everything worked, and I successfully created and organized a series of pages into a hierarchical "About Us" section for a fictitious company, including sub-pages for various types of contact information. I created several node new node types and included them in a book, and then removed them from the node hierarchy. Everything worked as expected; nay, worked much better than expected. I created books within books, even, and had no issues other than a small undefined constant notice in the 'printer friendly' view with regard to LANGUAGE_RTL -- I mentioned this to pwolanin in #drupal.

This is a significant increase in functionality and dramatically improves the book module. That said, this expands book module to a point where it almost seems as if the book paradigm is lacking itself; I mean to say, the functionality here is leaving the old book metaphor behind quickly. I found myself wondering if the entire module should be renamed into something more generic.

Warning--feature creep that should not hold up this patch follows:
I also wonder about how, or even if, the UI could be further integrated with the functionality exposed by menu module on the node add/edit screens. In some very real ways, book module, menu module and path module all provide some outward facing bits that work together to do some very nifty site structuring. I wonder if there is a way to combine the interface of all three into an enabled-by-default, holistic approach to creating hierarchy/menu links/paths.

pwolanin’s picture

FileSize
68.54 KB

the RTL problem is, I think, not due to this patch, but is a bug in the existing code. However, attached patch fixes this by checking if the locale module exists, since the constant is defined there (why not in locale.inc?).

I think that when this patch goes in, integration with the path module would be a good next step. I'd like to be able to see each node in the book hierarchy have a path "fragment" so that a full path can be made of the fragments from a given node to the root.

I think the menu and book modules serve different purposes (see #63 and #83), and so is it does not make sense to try to combine them. However, both do use the menu system hierarchy in {menu_links} (with this patch).

hunmonk’s picture

Status: Needs review » Needs work
FileSize
67.69 KB

the following query has more values than there are columns in the new book table, so the database update fails:

db_query("INSERT INTO {book} (mlid, nid, book_id) VALUES (%d, %d, %d, '%s')", $book['mlid'], $book['nid'], $book['book_id']);

attached patch corrects.

with that adjustment, both the new database schema and the database update (with or without existing book nodes) work in postgres.

there seem to be some UI problems with the patch IMO:

  • the 'Book outline options' collapsible fieldset is in a very funky place in the node page layout -- between the title and the body. it should most likely be down with the other fieldsets below the body.
  • it seems poor design to have both that fieldset and an 'Outline' tab, which only has the same stuff as the fieldset. we should pick one or the other.
hunmonk’s picture

FileSize
67.68 KB

whoop. that bad query i listed above actually happens twice. attached patch fixes the one i missed...

Jaza’s picture

Status: Needs work » Needs review

The patch as it currently stands doesn't do a great deal, AFAICT, apart from changing the book module to use menu items for storing the parent of each node. As far as I'm concerned, the patch falls way short of the proposal in http://drupal.org/node/128731, for a new "core hierarchical page structuring module".

I hate to be whining, and I hate to demand stuff that may be considered feature overkill for this patch, but I think that the following big changes are needed in this patch before it can go into core:

  1. Get rid of the dependency on nodes. Completely! Make this the dream outliner module that we so desperately need in core. This patch is halfway there - with it, the book module now defines page hierarchy through menu items. Take the patch the rest of the way. Allow ANY menu callback (i.e. any page - be it a 'system-generated page', a node, a taxonomy term, a view, or whatever else) to be outlined using the book module.
  2. For all non-node pages, add an 'outline' tab, allowing those pages to easily have their parent and weight (and title?) redefined (perhaps exclude it from admin pages - might not be appropriate allowing those to be re-organised so easily).
  3. For node pages, add the 'outline' feature as a fieldset in the node edit form. Make the feature toggleable per node type (like comments, attachments, etc). I vote keeping the 'add child page' link (for selected node types) - but instead of linking directly to the 'node add' form, make it first link to a page where the user chooses the node type of the child page, then goes to the appropriate form for creating that child node.
  4. Really, we don't need the {book} table at all anymore. Get rid of it. Just use the menu system for all the storage needs. Book module becomes purely a front-end to improve usability. 'Menu trees' become the new 'top-level books', and 'menu items' become the new 'book pages' - the entities are basically equivalent, so there's no need for the book module to continue having its own system for organising things.

And the following proposals are probably feature bloat for this one patch - but I'd definitely like to see them go into core at a later stage:

  1. Create a new generic library of hierarchical tree functions (in a new tree.inc file), and make the menu system depend on this library, instead of using its own code to do parents / children / prev / next / etc stuff.
  2. Remove all hierarchical structuring abilities from taxonomy.module, and instead use the new book / outline module and the menu system to organise taxonomy terms, same as for nodes (hey, they're pages, part of the menu tree, just like nodes, right? they don't need their own hierarchy system!).
  3. Change comment.module to utilise the generic tree library as well. Then, whereas we now have FOUR separate hierarchy systems in core (comment, book, menu, taxonomy), we will instead have only TWO systems (comment and menu), both using the same ONE underlying function library!

Would love to help make all this a reality, but since I'm travelling at the moment, my feedback will have to do for now. Hope it helps. :P

pwolanin’s picture

Thanks for catching the problem with the update code - an earlier version did have 4 columns and I must have missed that query.

The fieldset is in the same place in the node form as the book UI elements in 5.x.

Having the outline tab is very important for this reason: users with the "outline" permission can then organize posts that they do not hve permission to edit. I can think of many good use cases for this, and again, this is the way the module working already in 5.x.

catch’s picture

fwiw, I really really like the "outline" tab, and never got on well with the extra fieldset. Trying to put different content types in books, and having the form in two different places when moving them around has been really confusing sometimes. And as indicated by Jaza, would provide a consistent ui if taxonomy terms, views, panels etc. are ever book/outline-enabled.

pwolanin’s picture

@Jaza - yes, the main point is the conversion to the menu API and the removal of the hard-coded 'book' type. These are important steps, and if you look at the patch, they are clearly a major change to the code. This change gives a single place (menu.inc) to focus our efforts on optimizing the performance of handling of hierarchies of links. Then, other variants of this type of module can use the same API and benefit.

The menu module handles non-node pages and is also able to deal with multiple links to the same page in one or multiple menus. The book module gets its strength from being able to add those additional navigation elements, and a context-sensitive navigation block. The {book} table is essential, since this module has a single hierarchy - and by relying on this fact, the navigation elements are possible.

hunmonk’s picture

@pwolanin: the bottom line is that it's poor UI to have both the fieldset and the tab display at the same time. if you want to keep both, then you should find a way to only display one at a time depending on the circumstance.

hunmonk’s picture

spent some time looking over the code in this patch:

  1. i've seen some references to: MENU_MAX_DEPTH -- which i believe is currently 6 layers deep. does this mean that books will be limited in depth to 6 layers? that might be fine, but i thought it was worth bringing up.
  2. function book_form_node_delete_confirm_alter(&$form, $form_state) { -- this whole function seems a bit weird to me -- isn't there a more elegant way to accomplish this? i would at least think we should use a drupal_set_message() there instead of a form item.
  3. book_node_types() -- seems like this function could be accomplished more efficiently with a single query than going through the book_get_books() dance.
  4. i may have to plead ignorance here, but aren't there any FAPI 3 helper functions available to help w/ the AJAX callback? it seems weird that you have to set the cache by hand.

this is one huge patch -- i only made it about 1/3 of the way through the code before i ran out of gas! hope the above helps...

pwolanin’s picture

In the menu table split work by chx and I, MENU_MAX_DEPTH was set equal to MENU_MAX_PARTS == 6. However, after a conversation with David Strauss on IRC on optimizing the indices, it's clear that we will be able to extend MENU_MAX_DEPTH to a larger number without a performance penalty. However, your point is well taken that there will be a fixed maximum depth for both this and the menu tree.

On the AJAX, I spent an hour with Eaton in IRC to figure out how to make this work, and from him, this is *the* way to get AJAX to work with FAPI3. There are not any helper functions, but it he would be very happy for someone to write them ;-)

The result of book_get_books() is cached in a static variable and used later in the form building, so I think calling that is (on average) cheaper than doing a separate query.

Perhaps the deletion API will offer a more elegant way to alter the confirm form?

hunmonk’s picture

The result of book_get_books() is cached in a static variable and used later in the form building, so I think calling that is (on average) cheaper than doing a separate query.

yes, i did see the static var. i was more meaning that when you _do_ build that var, it seems like you could do some kind of 'SELECT DISTINCT type FROM..' query to get the types, instead of calling that other, more involved function. not a big deal, i just thought it might be a little quicker for the initial building of the static var.

pwolanin’s picture

Status: Needs review » Needs work

needs to be re-rolled for Deletion API

pwolanin’s picture

Status: Needs work » Needs review
FileSize
69.87 KB

initial deletion API update, and also updated install code to deal with 5.x orphan nodes.

pwolanin’s picture

FileSize
71.32 KB

deletion API code could be simplified a little if this patch gets in: http://drupal.org/node/153931
Attached patch improves the JS, so that scrolling through the select with the keyboard works as well as using the mouse.
Also, changed forms to follow this new FAPI3 imperative: http://drupal.org/node/144132#op
separates out each button's submit handler to a separate function. Also adds a confirm form for removing a post from the outline.

update code still needs a little more work to handle orphans properly.

pwolanin’s picture

FileSize
72.93 KB

code for handling orphans during updates now works in my initial tests.

Leeteq’s picture

Ref. #94:
It might be that the practicality of having both outweighs the "bad UI" concern in this case.

Anyway, if there is consensus for absolutely having it in only one place, I'd say a +1 for keeping the outline tab. Makes it logical and easy to spot for new users too.

But more importantly:
- would keeping this functionality in the outline tab actually make it possible to restructure book outlines without affecting the update timestamp of the pages? It is quite important to have the option of doing a restructure without having the affected pages pop up on top of the activity or recent listings.

I agree with most of Jazas points in #96, particularly regarding going all the way and making this totally independent/generic.

pwolanin’s picture

yes - right now the outline tabs allows users without edit permission to restructure the book, and such restructuring does not touch the node's timestamp.

pwolanin’s picture

FileSize
72.98 KB

Thanks to suggestions from Karoly, I improved the forms which also allowed the JS to be simplified a little as well.

Also use drupal_json() rather than directly calling drupal_to_js(), make the block title a link, and make sure every function has a doxgen comment.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I tested on Peter's site and I say it's ready. We can always tweak it more -- but it's very, veyr good now.

pwolanin’s picture

FileSize
73.03 KB

final, very minor, UI tweak from dww: if <none> appears in the select as an option, it should always appear first - above, not below, <create a new book>

dww’s picture

+1 on the RTBC. I gave this patch a medium-detailed review, and didn't spot anything wrong. I clicked through the test site extensively, tried many operations as different users, etc. All works better than I expected. Of course, we could probably tweak things here and there, but this is a vast improvement over the existing book functionality, so let's get this in.

Also, I think making the block title a clickable-link to the root of the book is a great solution, even though we don't normally make block titles links. The alternatives are ugly and gross, and this is nice and elegant, IMHO.

Finally, I agree that this should be renamed to something like "outline.module". However, that's a completely separate issue. 5.x's book.module is just as poorly named, so there's no reason to hold up this patch any longer over this. All of you who complain about the patch being too large should take notice: doing the rename *and* the port to menu *and* the nodeapi() changes, etc, etc, would make this even larger and less likely to be carefully reviewed. So, let's not lose site of reality, here. These changes are great, book is (still) a poor name for this module, and if we reward all of peter's hard work by committing this, we can handle the rename as a follow-up issue.

Thanks,
-Derek

p.s. I also respect Jaza's input on this, but I think that we have to take incremental steps here. This patch/issue already has trouble getting serious input since it's doing a lot. It can't really be made much smaller, but we can certainly prevent it from mushrooming even larger by not throwing every wish-list item and its best friend in here. ;) Again, if this lands, it makes it easier to do the complete outliner solution that Jaza was asking for, not harder. The patch for that would be relatively small compared to the initial foundation of menu-based-books laid here. Worst case, we do completely generic outlining in D7. At least we made book much better in the meantime, and the d.o handbooks won't be such a huge pain to manage.

pwolanin’s picture

for those not on IRC - temporary demo site: http://book.wolanin.net

catch’s picture

I had a look 'round the demo site last night. The UI is much better, everything works OK as far as I can tell. Any gripes I might have are inherited from the 5.x book module, not introduced with this patch, so agree with other posters here that it should go in now, with any extra tweaks/renaming etc. in a different patch.

joshk’s picture

This is looking nice!

I recommended that the "Book this page belongs in" be moved to the top of the fieldset rather than the bottom. I like it a lot better there.

pwolanin’s picture

FileSize
72.92 KB

re-roll since arguments to function db_create_table() changed and that breaks book.install code: http://drupal.org/node/150210

pwolanin’s picture

Note: pages are not necessarily sorted correctly by title in the block, etc. because there is a bug in menu.inc that is fixed by this separate patch: http://drupal.org/node/151583

Dries’s picture

Status: Reviewed & tested by the community » Needs work

1. The patch outputs things like "Submit Story" -- should be "Submit story".

2. I don't think this patch makes it easier to organize pages in trees. As I mentioned a dozen times, I'd like to get rid of the 'book' notion. I want to assign pages to a parent, not to a book and its parent.

3. I'm not sure I like all the Javascript going on. All of a sudden there appeared to be a button? I didn't manage to get the button back, but I know I was confused for a second.

I think I prefer to wait for a patch that does a better job integrating with the menu system and that manages to eliminate the concept of books. This patch could be simplified to make it easier for people to work with pages and hierarchies.

pwolanin’s picture

@Dries - Right now, this patch is not trying to be anything beyond an improvement of the book module to eliminante the hard-coded type and integrate with the menu system to use that API to store the trees of links. It's clear that I'm extremely stupid and unable to grasp what you're looking for as a more generalized module, so can we just discuss this patch in terms of a marginal improvement over the current book module with a couple small new features and potentially better performance?

Without this patch, or some similar improvement to the book module, the docs team will not be able to effectively re-organize the d.o handbooks to split 'official' and 'unofficial' docs as sepeck wants. Yes, a more 'generalized' module would be great, but the Drupal project also still needs a module that handles 'books' well.

pwolanin’s picture

Title: make book module into a better "outliner" » improve book module: use nodeapi and menu API
Status: Needs work » Needs review

the text 'Submit Story' has nothing to do with this module or patch. This is coming from node_add and wipes out the title set in node_menu

The only button that gets hidden by the JS is one on the node form that acts basically like the 'Preview' button and serves to update the second select if JS is not present.

Changing title as I should have done at about #63 above. The current patch proposes improvements to the book module. period.

pwolanin’s picture

FileSize
71.99 KB

re-roll for this patch: http://drupal.org/node/153364

catch’s picture

Like I said, I tried it out, and it's a big improvement in UI (not to mention the back end stuff which I get the idea of but can't review how that's coded).

There's other stuff that could be done to change 'book' into an outline.module - but that's not going to get into D6, this can, and it's a prerequisite to further abstraction.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Dries, the patch is a big step forward. Other steps can be made later. If you want to have something that organizes nodes into a hierarchy this works, and much better than we had earlier. Menu is a diffrent thing.

dmitrig01’s picture

@Dries - as has been stated earlier, this patch simply uses the node api and the menu api how they should be used. It does not change any concepts or re-name anything. That is for another patch entirely. This patch simply brings book module up to date to use what Drupal provides it, not hack up its own solutions.

dww’s picture

I still support this patch (and the new title of the issue). Our motto should be "code is gold". The patch represents probably hundreds of hours of work bringing book.module up to date with our existing core APIs and giving it some much-needed usability improvements. Is it perfect? No. Will we ever improve upon it? Of course. Are we going to go 2 major releases of core without improving book.module at all (dogfood we eat on d.o itself)? I certainly hope not. Should book be renamed and undergo yet more changes in the future? Definitely.

As with the Deletion API, let us not waste this immense effort because someday something else might supercede it. In the name of "it's not perfect yet," we must not paralyze ourselves and prevent making progress at all. The drop is always supposed to be moving. Let's not leave book behind.

pwolanin’s picture

FileSize
75.79 KB

re-roll for new hook_help params patch. Thus, minor changes in the patch just to function book_help().

pwolanin’s picture

After a discussion w/ Jeff Eaton on IRC, here's my attempt at a clear explanation of what this patch actually does now:

  • Removes the hard-coded 'book' type and perform all node actions equally on any node type via hook_nodeapi
  • Achieves 100% integration with the menu system.
  • Stores each book as a menu (i.e. a hierarchy of menu links), and does not maintain a hierarchy at all in the {book} table.
  • All the algorithms have been changed to use the tree data structure returned by the menu system.
  • Breadcrumbs are set automatically
  • The UI is changed, in part, because there is no fixed node type - for example, the 'Outline' tab is always present.
  • The UI is also changed to make it easier to move a node to a distant parent - only the pages in one book are shown at a time, as well as a select to choose a different book. (This also results in a performance improvement, or at least less memory demand.)

    Important note- the new D6 menu system is totally independent of the (now optional) menu module. This can be confusing, since many of the function names were carried over from D5, and they still share the menu_* function name prefix (perhaps this should be changed). In menu.inc is an API for storing an manipulating hierarchies of menu links. Menu module is one interface to that API, and this patch makes the book module another one, but with different assumptions/constraints.

    Also, note that the UI changes are a small part of the overall patch, which is fundamentally a movement of this module to use the nodeapi and the new menu system. I realize that it's easy to get hung up on the UI, but please look at the code.

nedjo’s picture

Status: Reviewed & tested by the community » Needs work

Comments on the Javascript only. I haven't tested.

This is nicely coded and looks to accomplish in a few simple lines what other attempts I've seen took much longer to do.

Some points for refinement.

1. The behavior should have a separate attach function rather than be added directly to the jQuery ready object. This facilitates reattachment.

2. Drupal.bookFillSelect should not be part of that function but, for consistency with what we do elsewhere, should be defined independently.

3. The behavior is generally useful and so ideally wouldn't be hard-coded to book.module. The JS is written well enough to be relatively easily genericized to e.g. updateSelect. Probably we should do this through the Forms API, setting element properties and a #process callback.

4. The issue of attaching behaviors to a collapsed fieldset is not one I was aware of and may require more thought and a fuller solution. What would happen if someone form_altered the book fieldset to set ['#collapsed'] = TRUE? It looks like that would break the behavior. We have the anomalous situation where Forms API can only say ['#collapsed'] = FALSE, while the JS hard-codes ['#collapsed'] = TRUE. A possible solution is to add the JS in a #process callback, where we can also set #collapsed to FALSE and send a setting indicating the original state of the fieldset (collapsed or not), which the JS then reinstates. Ideally we'd find a way to solve this for all behaviors at once, since presumably other behaviors will also fail to attach to elements in collapsed fieldsets.

Points 1 and 2 are quick and easy fixes.

Points 3 and 4 obviously would require more work and arguably could wait for followup patches. Point 3 might benefit from the forms AHAH patch currently in the RTBC queue.

nedjo’s picture

Status: Needs work » Reviewed & tested by the community

Points 1 and 2 are too minor to hold up the patch.

Points 3 and 4 are longer term issues.

add1sun’s picture

Status: Reviewed & tested by the community » Needs work

I'll just speak to this quickly purely from an end-user, handbook maintainer perspective. This isn't the "holy outliner module" that we ultimately want and it is certainly not even a perfect book module, but it is an improvement for book module as it currently is. I would REALLY like to use this in D6 and not wait another year for book/outliner/whatever to come along. This patch simply brings book up to current with the rest of Drupal. Things that I like about this from the end-user view:

1) Using nodeapi so I can just outline my content, whatever it may be. This makes it feel like it belongs in a CCK Drupal world rather than the old school hard-coded world and I'm not confused by the book content type when I have my own ideas of what my books should be made up of.
2) Limiting the selection to one book at a time. Especially on sites like d.o this is a HUGE improvement on UI, maybe not perfect but the current UI is ridiculous and overwhelming.

So, seriously that is all I'm looking at here, not having a chance to really review the code. Is that enough to put it in core? That's up to Dries but I do feel that it is worthwhile for the end-user experience.

Frando’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
75.82 KB

The attached patch fixes the two first issues nedjo mentioned. The other two issues really do not belong here, they should be follow-up patches.

yched’s picture

add1sun cross-posted with nedjo, and reading her post, I don't think she actually meant to set back to 'needs work'

chx’s picture

Remove the "book page" node type, and turn the book module into a module that is specialized at creating hierarchical content trees. The module should automatically extract a menu from the node hierarchy, set the correct breadcrumbs, help you define a hierarchical URL structure with clean URLs, etc. This module will be the de facto standard to structure pages and to create hierarchical navigation schemes in Drupal.

http://buytaert.net/suggestions-for-drupal-core let's see, we have book type removed, it's creating node trees as ever just now via menu system, automatic extraction no (what's that anyways -- especially now a book is a menu), breadcrumbs are set, hierarchical URLs are not set but that'd be an easy followup patch. So, there the book patch was high on the list but now that you have something well tested, supported and implemented it's a no go :( ? As I always say, surely can be something better, but this is good enough in itself.

chx’s picture

important: http://drupal.org/node/146425#comment-253019 that's the documentation team lead 's comment.

Frando’s picture

FileSize
75.92 KB

This patch updates book.js to use the just-commited behaviors patch. Tested, no functionality changes, and confirmed to be done right by nedjo.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Stop talking about how multiple books would be useful. I know it would be useful. We are NOT debating whether we should implement this feature. We are debating HOW it should be implemented. Please, get your act together if you choose to respond. Thanks.

As I said repeatedly, there are many good things in the patch. However, it does some things that I disagree with.

Achieves 100% integration with the menu system.

At the API level, it might but not at the UI level. For this patch to be committable it needs to integrate properly at the UI level too. In other words, the module needs to integrate with the menu module (i.e. it should probably have a dependency on the menu module and offload a number of its tasks to the menu module). When I create a book, and when I add it to the menu using the menu module, the book's child pages are not added to the menu. Only the root is added. In other words, this patch provides integration with the menu system, but it works independantly from non-book pages. That seems flawed to me, and makes it rather confusing. It means that it fails to be "the tool" to structure pages.

Given the many important cleanups this patch adds and the fact that it brings the book module up to current with the rest of Drupal, I would be able to live with this flaw assuming that we'd fix it in Drupal 7. However, what makes it non-committable is the fact that this patch introduces new functionality that is implemented in the wrong way, and that carries us away from proper menu module integration. The support for multiple books is useful, but it stretches out the flaw. Committing this to Drupal 6 would probably mean that we'd need to rollback large chunks of this patch once we start working on Drupal 7, and that we'll be presented with a challenging migration path that depends on menu.module changes. That's going to going to cause a lot of trouble. Committing this patch now, is going to lock us in in the wrong technical implementaiton and I want to avoid that at all costs.

So I'm afraid that this patch suffers from feature creep. It does a number of good things that I'd love to commit, but it also takes the book module in a direction that I don't agree with. The best way forward is to split this patch in smaller chunks, and to commit one chunk at a time. Step #1 should be to remove the book node type. Step #2 should be to integrate properly with the menu APIs. Step #3 should be to integrate with the menu module. Step #4 should be to improve permission support in the menu module/system so we can implement multiple books.

The current patch provides step #1 and step #2, but fails at step #3 and step #4. I'd love to commit #1 and #2 though. I could live with committing #1, #2 and #3, but only if #4 is left out of this patch. We can rework #3 in Drupal 7, but it's going to be troublesome if we need to rework both #3 and #4 in Drupal 7. The best way to get there is to remove the support for multiple books from this patch (#1, #2 and #3). Then for Drupal 7, we can figure out how to support multiple books with support from the menu module.

Hope that helps!

pwolanin’s picture

@Dries - integration with the menu module is not feasible in the short term. The menu module allows one to have an arbitrary number of links pointing to the same page. You cannot do that and still have book-module-like navigation (previous/up/next) unless you start playing tricks with the session, or maybe query strings, or something a bit ugly to track the 'context'. This is why book module still has a separate table - the information in this table provides a unique context from each node in the hierarchy.

I don't understand what is the #4 that *must* be left out - from your comment, it seems that you are unhappy that #4 is not implemented? The only permissions used in this patch are those defined by node module - e.g. node_access('create'), and to me were the most direct way to move to a nodeapi-based system from the previous 'book' node type.

dmitrig01’s picture

The only problem with Dries committing without #4 is that d.o uses multiple books, right?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
75.69 KB

re-roll of patch for Deletion API roll-back

@Dries. re-reading you comment. This patch only does #1 and #2 on your list, so I'm totally perplexed by the rest of the comment.

eaton’s picture

Status: Needs review » Needs work

At the API level, it might but not at the UI level. For this patch to be committable it needs to integrate properly at the UI level too. In other words, the module needs to integrate with the menu module (i.e. it should probably have a dependency on the menu module and offload a number of its tasks to the menu module). When I create a book, and when I add it to the menu using the menu module, the book's child pages are not added to the menu. Only the root is added. In other words, this patch provides integration with the menu system, but it works independantly from non-book pages. That seems flawed to me, and makes it rather confusing. It means that it fails to be "the tool" to structure pages.

Dries,

I can't comment on some of the other issues, as they primarily relate to the documentation team and their needs on drupal.org. (They've been angling for book.module improvements for a while, and I talked to sepeck a while ago about helping, but got sucked into FAPI stuff and wasn't able to dedicate much time...)

The trail you seem to be pointing down is the one that was blazed by Category module a couple of years ago. The expressed problem was that there should be one method for 'building hierarchies' that's used everywhere. At the time, the idea was that taxonomy, book, and menu should be merged into a single mega-module. Making book and menu use the same UI and mechanisms isn't quite that extreme, but edges in the same direction. I think it's compelling at an abstract level, but is a mistake in the real world of Drupal usage. I've argued before that the answer is to standardize on underlying APIs and storage mechanisms while allowing high-level modules to present situationally appropriate UIs. Why?

  1. Pure hierarchial site structure management, and managing online documentation/online books, etc, are different conceptual tasks with different UI needs.
  2. The refactoring in this patch, along with the refactoring in menu.module and the menu.inc APIs, move book.module's underlying storage mechanism to the menu APIs.
  3. The underlying API/storage refactoring makes it easier, not more difficult, to expose a streamlined UI in Drupal 7 that lets users manage the entire site hierarchy without the concept of a 'top level book'.

I've been on the fence with this patch for a while, but after spending a lot of time reading over your concerns (which I think are valid), studying the code and the API changes, and picking Peter's brain about the way it works, I think that it it accomplishes a subset of what you're hoping for without standing in the way of subsequent improvements that take us all the way to your vision for site outlining.

Because drupal.org depends so heavily on book.module, I think this approach is the best one. Obviously, it's your call, but I think that some of the history of this patch may be clouding the fact that as it stands now it is real progress along the path that you've laid out. Hope this analysis is at least helpful.

eaton’s picture

Status: Needs work » Reviewed & tested by the community

I've just taken it through its paces and the code itself is -- from a technical standpoint -- RTBC in my opinion.

I want to make clear that I think this is Dries' judgement call, and I don't want to play "It's ready"/"No, it's not"/"Yes it is" ping pong with the issue status. If you find the discussion above compelling and feel that it provides useful refactoring and enhancements for the doc team, without standing in the way of future outliner development, this patch is ready.

Dries’s picture

I'll have another look at this tomorrow morning. From reading the comments, it looks like we're just running around in circles though.

pwolanin’s picture

While this is pretty obvious, I just want to point out that the patch does not touch any files outside modules/book - it's using system API changes, not making or requiring any.

Also, this wish list for an improved book module:
http://lists.drupal.org/pipermail/development/2007-March/023188.html

nedjo’s picture

I'd like to find a better solution to the issue of handling collapse in the Javascript. What we have now is a hack.

I'd like to understand better why the behavior doesn't attach to a collapsed fieldset's children. Is this true for all events we'd like to attach?

I wonder if something like the following would be a general solution:

// Unhide any hidden parents.
var hiddenParents = [];
var parents = $(elt).parents();
for (i in parents) {
  if (parents[i].css('display') == 'none') {
    parents[i].css('display', 'block');
    hiddenParents.push(i);
  }
};

// Attach behavior.

// Rehide parents if needed.
if (hiddenParents.count) {
  for (i in hiddenParents) {
    parents[i].css('display', 'none');
  }
}
pwolanin’s picture

FileSize
75.71 KB

@nedjo - it's very odd. With the updated JS, the select-updating behavior works whether or not the fieldset starts out as collapsed. However, hiding the button in the node form only works with the original code (expanded in the FAPI code, then JS sets to collapsed). I tried every combination I can think of, without luck. I don't uderstand why the hide() should be different that attaching a function.

Attached patch is a trivial re-roll to account for this: http://drupal.org/node/142157

dww’s picture

Status: Reviewed & tested by the community » Needs review

More appropriate status, given the new patch and concerns about JS. Sorry I don't have time to thoroughly review and test right now. :(

nedjo’s picture

I don't uderstand why the hide() should be different that attaching a function.

Thanks for digging into this, I think you've hit on the key to the problem.

$().hide() will only set display: none on an element that is currently visible. (The method uses this.filter(":visible").) Since the button is inside a fieldset that is already not visible, no change is made.

So apparently all we need to do is explicitly set the css display property.

$('#edit-book-pick-book').css('display', 'none');
pwolanin’s picture

FileSize
75.71 KB

Thanks nedjo, I think the attached patch fixes the JS - seems to work fine regardless of whether the fieldset starts out collapsed or not. And as a bonus, the JS is shorter!

on IRC:

webchick: I think that book.module should come with a node type by default (editable/deletable) so that the module does something out of the box, as it used to do. See forum for example."

The previous patch only created the type if there was existing D5 'book' content, but the .install in this patch always creates it per Angie's suggestion.

treksler’s picture

Title: improve book module: use nodeapi and menu API » aren't we back at square one here?

Unless I missed something,
this patch now implements a book module, complete with a book node type, using the new D6 APIs, etc.

If the goal was to come up with the best way to implement the "book" paradigm, then this patch does an admirable job.
The trouble is that the task at hand was to remove the notion of a "book" ..............

It's July the fourth, where does this patch go from here?

I suppose it couldn't hurt for books to live on as a contrib module to allow for the handbooks to live on "as is",
but the bottom line is that Drupal STILL desperately needs a "de facto" way of building hierachical websites.

The taxonomy/menu/books/panels/comments/outlines/category module dance is getting really frustrating
... and merely having a better book module doesn't really address the problem at hand :(

treksler’s picture

Title: aren't we back at square one here? » improve book module: use nodeapi and menu API

whoops, didn't mean to change the title .. now there's a usability issue ... anyway ...

moshe weitzman’s picture

@irstudio - yes, you missed the whole discussion here. this issue is titled "improve book module". go elsewhere if you want to discuss something else.

pwolanin’s picture

In parallel, we are trying to improve the functionaity and UI of the menu module. So, I'd encourage you to look here and hekp imrove the menu module as the *default* site-structuring tool: http://groups.drupal.org/node/4850

pwolanin’s picture

FileSize
78.6 KB

After discussion on IRC, here's a modified patch that simplifies the permissions.

There is an additional permission 'add content to books'. Users with this permission can add the allowed types to the outline.

There is also a settings page where the site admin can determine which content types may be added, and which type should be used for the "Add child page" link.

Also, minor changes were made to conform to this patch: http://drupal.org/node/154469

pwolanin’s picture

Moshe and Dries had mentioned before wanting the "Add child" link to be able to offer a choice of all available node type. I just found this module which might serve as an example: http://drupal.org/project/quick_child

pwolanin’s picture

FileSize
78.66 KB

minor cleanup of book.install.

sepeck’s picture

working through the demo site, the interface looks and feels good. Things we're were I expected them.

moshe weitzman’s picture

i also worked through the demo site using 3 different users of differing permission levels. i reported 1 bug to pwolanin concerning the fieldset on node creation for regular user. i also asked for an admin link to be made more accessible in help text. otherwise, looks lovely to me.

pwolanin’s picture

FileSize
78.5 KB

Moshe found one bug - I had not fixed book_nodeapi($op='prepare') for the changed permissions - this manifested for users without 'outline posts in books' permission.
Also, Moshe suggested that a link is needed in the help at the top of node/%/outline to admin/content/book

Updated patch for these minor bugs.

moshe weitzman’s picture

Status: Needs review » Needs work

both minor bugs confirmed fixed.

a code review identifies only really minor issues:

  • hook_submit() is gone. change comment for book_submit() noting that it is a fapi submit hander on node form. you might consider using book_nodeapi('presave') insted though. would be a bit easier for people writing import scripts.
  • typos - search for 'repalce' and 'confrim'
  • would be nice to have an is_node_form() function which does this madness:
    if (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] .'_node_form' == $form_id). can be a different patch though.
  • the line below from _book_parent_select() looks odd. specifically, shouldn't the left hand side be $form[whatever]:
    + $form = array('#type' => 'hidden', '#value' => -1, '#prefix' => '<div id="edit-book-plid-wrapper">', '#suffix' => '</div>', );
  • perhaps reword: "Select the book you wish your page to be a part of." to "Add your post to a book."
  • is $node->build_node supposed to be useful for book.module, or did that part of the node render patch get axed?. eaton?
moshe weitzman’s picture

that last bullet was meant to be node->build_mode

pwolanin’s picture

Status: Needs work » Needs review
FileSize
79.7 KB
  • fixed doxygen comment for book_submit. Using 'presave' would actually make programatic saving harder, since it would overwrite the uid.
  • fixed typos
  • _book_parent_select() is written that way since it is called as $form['book']['plid'] = _book_parent_select($node->book); (instead of having to do an array merge).
  • added build mode NODE_BUILD_PRINT to node module, which allows the code in function book_node_visitor_html_pre() to be simplified and to match other core usage. Updated book_nodeapi('view') to check $node->build_mode.
  • Reworded 'Select the book you wish your page to be a part of.' to 'Your page will be a part of the selected book.'
moshe weitzman’s picture

i see NODE_BUILD_PRINT getting set in the _pre() function but where is that flag used?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

pwolanin explained that book navigation is now appended only if NODE_BUILD_NORMAL is set. sounds good to me.

this is RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I'm going to give this patch a bit of a work-out over the weekend. I just spent some time looking at the code, and I've some first requests:

1. It would be great if you could give a rough overview of what book_update_6000() does. I suggest you add two or three sentences to the PHPdoc of that function. This is minor, but it helps us understand that update function 6-12 months from now.

2. Can be rename the CSS class/ID 'bookSelect' to 'book-select'? We don't use camel casing in CSS classes/IDs?

3. modules/book/book.js has no documentation -- it would be nice if you could describe each behavior in one line.

4. I have a hard time remembering what 'plid' stands for. We're now exposing that term to the CSS which might confuse designers. (I know it confuses me.)

5. We now have a "add content to books" and a "outline posts in books" permission. I'm afraid that the distinction is not clear. I'd like to see us think about this some more, and maybe rename these permissions, merge them, or whatnot. This is my biggest gripe at the moment.

6. "AJAX callback to replace the book parent select." -- We could be a tad more verbose here. Not everyone might know what 'book parent select' means.

7.

 +  // We do not want to execute button level handlers, we want the form level
+  // handlers to go in and change the submitted values.

Would be nice if you also mention _why_ we want this.

8. "Like a node preview, to update options in the parent select." is rather abstract.

9. "Common helper function to update the book outline." -- what is meant with 'updating the book outline'?

10. "Update the book_id for a page and its children when we move it to a new book." -- this is quite techno lingo for API documentation. The $link parameter is not documented either.

11. + static $flat = array(); if (is_null($book_link)) {print("hello");} Debug code?

12. $type_obj -- we don't abbreviate variable names like that. Just write $type.

13. Why 'book_id' and not 'bid'? bid would be more consistent.

14.

+      $expire = max(ini_get('session.cookie_lifetime'), 86400);
+      cache_set($cid, $form, 'cache_form', $expire);

Why are we using the cookie_lifetime here? That's seems a little peculiar. (Just document that in the code.)

15. Get the data structure representing a subtree. the root item will be the ... -- incorrect punctiation and techo lingo.

16. A fully loaded menu link, -- typo: should be point, not comma.

17. book_node_type() lack documentation.

It's hard to review this patch. I just spent another hour with it, and still I don't grok all the aspects. The PHPdoc is spare and it makes a lot of assumptions -- it assumes that the reader is neck-deep into the book module already. I know a fair bit about the book module and yet, its documentation doesn't really help me.

It would be great if we could make the PHPdoc a bit more complete and if we could keep in mind that the PHPdoc is meant to teach people about the inner workings of the book module, and its functions. Often the PHPdoc doesn't help me figure out *why* you'd want to call this function or *when* you'd want to call this function. I like it when just reading the PHPdoc gives me a good grasp of what the module does, how it does it and why it does it like that. The current PHPdoc of the module doesn't get me to that level.

This is not a show-stopper, it just complicates the reviewing as it takes me significantly more time to connect the bits and pieces in my head.

pwolanin’s picture

@Dries - thanks for the feedback - I 'll get some of the this done immediately, and continue with the rest.

a couple quick notes:

the permission "outline posts in books" is the existing permission so I was a little reluctant to change it, but it would be more accurate/distinct to call it something like "administer posts in books" or "administer book outlines"

I used book_id rather than bid for the column because bid is already used in the {boxes} and {blocks} tables and I personally find it an impediment to understanding the schema when the same index name is used for totally different things.

plid is "parent link ID" in parallel to mlid "menu link ID" - both of these are from the new menu system. This shows up in the CSS since there is a 'plid' form element so that the form values can be used directly to make the menu link.

yched’s picture

"bid is already used in the {boxes} and {blocks} tables" - and in the, er, {batch} table...

catch’s picture

Just a quick vote for "administer book outlines" as the permission name.

pwolanin’s picture

note, this code comment:

+  // We do not want to execute button level handlers, we want the form level
+  // handlers to go in and change the submitted values.

is actually verbatim from the node module in 2 places (functions node_form_submit and node_form_build_preview). So I agree it's not very clear, but should perhaps be updated in node module as well.

pwolanin’s picture

FileSize
81.94 KB

The changes in this patch are almost all increased/improved code comments, plus removed that one piece of debug code, changed CSS class to "book-select-processed", and renamed a couple variables (e.g. $type_obj to $type).

dww’s picture

I'd rather have more clear variable and column names (book_id) than a little bit more phpdoc to tell me that "bid" in this module means "book id". @Dries: your request about "I have a hard time remembering what 'plid' stands for." seems to contradict your one about bid. pwolain can't change all of the terse, hard to remember variable/column names at once in this patch, but at least the new ones he's adding can be a little more self-documenting.

That said, I'm certainly a fan of clearly documented code, and figured that a seriously well-commented update.module was the only chance I had of getting it in core. Seems to have worked. ;)

Finally, "administer book outlines" also seems sane/good to me. I immediately understand what that's supposed to do, and it's clear to me how that's different from "add content to books". Renaming the existing permission is ok. If you want some help with the DB update path, take a look at signup_update_4() from signup.module, where I had to rename a bunch of confusingly named permissions.

Cheers,
-Derek

pwolanin’s picture

FileSize
84.46 KB

working more on doxygen comments, including adding "@ingroup forms", and @see as recently recommended.

Also changed the permission name to "administer book outlines" in the module (not yet in .install), and altered function book_admin_overview() to use book_get_books() rather having its own query.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
86.08 KB

added the code to book_update_6000 in book.install to update all the permissions. Tested and works.

Minor additional doxygen fixes.

Also, altered book_outline() so that the form building function does not call drupal_set_title() (changes one entry in hook_menu - do a menu_rebuild by enabling/disabling a module).

Dries’s picture

Thanks for the quick updates. I'll review this patch in the next 24 hours, though feel free to upload new versions or to solicit feedback from other people. A 10-minute pass over the latest patch shows that the code comments got significantly better. Good job, and much appreciated. :)

re: 169 -- ah, right! Time permitting, feel free to improve the code comments in the node.module too.

re: 171, dww: well, but bid is more consistent with the rest of Drupal, and if the module is called book.module, it's actually quite easy to guess. ;-)

dww’s picture

I stand by my support for "book_id". WTF is a "vid"? Depending on what module you ask, it's wildly different things. :( It might be a Vocabulary ID, it might be a reVision ID, etc, etc. Just because the rest of core uses the way-too-short-to-be-useful variable/column names doesn't make it right. Self-documenting code is more useful than well-commented, cryptic code. I'll grant, in this particular case, "bid" is pretty clear, but I don't buy the "but the rest of core does it" argument in this case.

IMHO, this is something that cries out to be improved core-wide (just like the namespace collisions for the $node object caused by much the same problem -- e.g. WTF is a "pid"?). We can't fix all of it in this patch, or even in D6, but there's no reason not to make an incremental improvement here. ;)

pwolanin’s picture

FileSize
87.8 KB

minor updates to code comments (including node module) and some of the strings on the book admin page.

@Dries - I really don't like having the same column name in many unrelated tables, so I don't think changing book_id to bid is a good idea, but something unique but shorter like bkid would be fine.

Jaza’s picture

This patch is looking much better. Haven't had time or testing environment to try out the patch properly, but I had a look at the latest patch, and have some feedback:

  1. Why does book.module still need to generate blocks for navigation? Aren't these now redundant, since every book page has a menu item generated for it, hence navigation blocks are now available courtesy of menu.module?
  2. This may be outside the scope of this patch, since it still desn't provide proper support for multiple books, but it would be good if we could make the book.module permissions per-book / per-menu, rather than global. E.g. instead of "edit book content", we could have "edit content in book xyz".

I'm definitely part of the majority now, who are saying "getting this into D6 is much better than getting no book.module improvements into D6". Hopefully it can still make it - what we have here at the moment will make a big difference, IMHO.

Dries’s picture

Spent some more time testing this patch.

1. The fieldset should be called 'Book outline' instead of 'Book outline options'. The word 'options' is redundant and the other fieldsets don't use it eiter.

2. The name 'book outline' is a really bad name, but as long we chose not to changes this module for the better and give up the book connotation, it's the best we can do, I'm afraid.

3. "This will be the top-level page in this book" should end with a point.

4. "The book to add this post to:" is not consistent with "Parent item:". They different in tone, the titles should not be instructions but just descriptions.

5. Personally, I'd prefer to merge both fields again. When I change the book, the parents change by means of AJAX. This is somewhat confusing as it's not very visual; it happens under the hood.

6. Having support for multiple books might lock us in. In the future, we'll want to integrate this closer with the menu module. In that case, I don't really care about the 'book' a page belongs to -- I only care about the parent menu item. Specifically, I should be able to start a new book under the existing about-page, for example.

I'm afraid that the current system takes too big of a step forward to the point that it will be too hard or impossible to undo this change. It's easy to add features but it's hard to take functionality out again. While the code looks good, and while the goals are useful, I think we failed to get consensus on how this should be integrated with the rest of Drupal. While there is nothing fundamentally flawed, I'm afraid it takes us into a direction that we'll regret later. I still think that integration with the menu module is key and that getting rid of the 'book notion' is crucial for wide-spread use of this module.

I'd like to brainstorm a bit about the future of this module, and how we could migrate from the proposed module to a version of the module that is easier to use, that gets rid of the book mantra, and that allows me to seamingly structure pages (and have navigation links be automatically generated). Imagine this gets committed to Drupal 6, what are the next steps for Drupal 7?

pwolanin’s picture

@Dries - In the menu module I got the single select with all possible parents working again because I know you prefer that UI for that module, but I think that there is general support for the separate selects as better for the book module because it improves usability when there are thousands of links.

The way I see it, I don't think this change locks us in except that we are committed to using the menu tables for hierarchical storage instead of the book table. What's changed about the books in this patch is not really that books are more separate, but rather that we have direct access to the information about which node is in which book.

For D7 to have the menu module add prev/next navigation elements would require some fundamental changes: in particular, any given link could only appear once per menu (now it's not restricted) and we would need to add in Drupal some way to add these navigation elements onto non-node pages (or they could be in a block whose placement is chosen by the theme/admin). I think a good bit of code from this version of the book module could be adapted to this, but adding non-node links into the mix makes it a task requiring some different approaches.

Please also look at Jeff Eaton's comment in #141 again too.

I'll make the changes you suggest above, and I'm also thinking that Moshe' suggestion of removing book_submit() in favor of book_nodeapi('presave') makes sense if we do away with the re-assignment of the node's uid on save (which currently is causing headaches for the d.o handbooks).

catch’s picture

single select.

I don't use the book module for navigation any more for this reason, however, when I did:

History
-Asia
--Japan
---Taisho Period
----Rice Riots 1918
-----Some book
------Chapter 2
------Chapter 3
------Chapter 4
-----Osaka
------Chapter 1
------Chapter 2
--Korea
--China
-Africa
-Americas
-Europe
-Oceania

But with about 1500 nodes and more depth than that. This means a massive amount of scrolling up and down a smallish select menu, working section by section, scrolling past, scrolling back etc. etc. unable to use keyboard shortcuts to get where you want.

It's an absolute nightmare to navigate, as is the book menu select on drupal.org

The two select method cuts out a massive amount of that scrolling and eye-strain, and will make it much easier for me to maintain the area of our site which still uses book.module (now a couple of dozen full-length books with up to 35 chapters each and a smallish 'handbook').

The current ui for organising books is broken IMHO, the version in this patch is a massive, massive improvement. Even if it lasts for only Drupal 6 and is superceeded by something more drastic, I think it will make a big difference, both to me and others with relatively large numbers of nodes in the book module, and to Drupal.org itself.

pwolanin’s picture

FileSize
86.46 KB

@Dries -

  • Updated the problem text you highlighted.
  • improved the JS so now it uses a progress bar to make more evident the fact that the parent select is being replaced.
  • confirmed with Addi that forcing the uid to change is undesirable (at least for d.o), so book_submit() is removed and instead $node->revision is set in book_nodeapi('presave').
pwolanin’s picture

FileSize
88.01 KB

when rolling the last patch I forgot to include node module (for the added build mode).

BioALIEN’s picture

@#180
catch, I couldn't agree with you more.
pwolanin, I'm loving the enhancements made by your patch. Well done!

pwolanin’s picture

FileSize
88.1 KB

minor fix thanks to webernet's testing - the 'presave' op in book_nodeapi was missing its break;

Also, via testing of this patch, I found a bug in the menu code: http://drupal.org/node/162095

pwolanin’s picture

FileSize
88.81 KB

More feedback from webernet - fixed a bunch of minor punctuation or formatting issues for the code comments, fixed a very minor bug in function book_admin_edit_submit() (the book title should be re-loaded for the message since it may get changed by the form submission), and changed the permission 'see printer-friendly version' to 'access printer-friendly version' to be consistent with all the other core permissions like 'access content', 'access comments', 'access user profiles', etc.

webernet’s picture

After some further testing, most of the bugs/issues that I've been able to find have been corrected in the latest patch. (There is one relatively minor bug that pwolanin is aware of and is looking into involving some strange, intermittent, behaviour of sixth level items.)

pwolanin’s picture

FileSize
89.08 KB

minor code comment changes and formatting suggested by chx.

chx’s picture

So I read the whole patch and there were a few things that made me go "uhm" (esp. the use of each without a reset -- I had no idea that assignment comes with an array pointer reset) but those are commented now. I also was wondering whether menu_tree_all_data in a block will cause performance hit -- not because it's cached and even without that, in this case the query strongly resembles the one for a normal menu, so all is good. Next, there is a JS form -- that's a beauty, Drupal 6 features well used, it could be the example of how you do dynamic forms w/ D6 (it's the example in the book :P ). Note that we now have a $node->book which nicely keeps together book properties. All in all, what I see is a very nice use of all new D6 goodness -- I do not have any problems with how this coded.

pwolanin’s picture

note- the intermittent bug webernet is referring to is in the sorting or rendering code in menu.inc, not in this patch.

flk’s picture

Status: Needs review » Reviewed & tested by the community

I really like the fact of how this is independent of the menu.module and makes its own menu structure opening up quite a few interesting possibilities.
With all the main problems fixed this patch to me it seems this patch is ready be committed

flk’s picture

And yes I did have a look at the code and tested it on my test server.

Stefan Nagtegaal’s picture

Status: Reviewed & tested by the community » Needs work

+ $('#edit-book-pick-book').css('display', 'none');
should be:
+ $('#edit-book-pick-book').hide();

Wouldn't it b e nicer if we change:
+ pb.setProgress(-1, Drupal.t('Updating parents...'));
to something more descriptive? (can't come up with something myself, but I think it could have a better progress message.

Further more, I like the functionality the patch is giving us.
Though, some questions:
- Why do we need the "new" node type 'book'? Is that neccesary for backwards compatibility, or do we just want another type to be immediatly associated with the book outlining structure?

catch’s picture

book is necessary for backwards compatibility I think.

Dries’s picture

I just started testing this patch some more.

As user #1, I created a book with 3 pages and enabled the "Book navigation" block. However, no new block is showing up. This is confusing, and might be a bug?

I've to run for lunch now so I can't debug it myself. However, I figured I'd already report it. I should be able to test this some more this afternoon.

pwolanin’s picture

@Stefan Nagtegaal - if you scroll back you'll see that the use of $('#edit-book-pick-book').css('display', 'none'); is necessitated by the fact that the element we're hiding is inside a collapsed fieldset.

@Dries - the book navigation block should only appear when you are viewing a node that's in a book. On other pages it does not appear - this is true in 4.7.x, and 5.x also.

pwolanin’s picture

FileSize
90.66 KB

per discussion w/ Dries and chx, changed the patch to:

  1. convert the column name 'book_id' to 'bid'
  2. add a feature so that the book navigation block has two modes. The new default is to display all books, the alternate mode is to display the current book's menu only when viewing a page that's in a book.
pwolanin’s picture

FileSize
91.66 KB

Updated the block settings description with input from chx and sepeck, as well as whitespace cleanup, etc. Also, chx suggested using 'Book navigation' as the default block title.

pwolanin’s picture

Status: Needs work » Needs review
Dries’s picture

Committed to CVS HEAD. Thanks a ton, pwolanin. Good job.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks a ton, pwolanin. Good job.

drewish’s picture

pwolanin, that this got committed is just proof of your indestructible will, i'd have given up on it long ago. congratulations.

taqwa’s picture

I'm sorry. I hate to do this, but you guys seem to know the book module really well. Could you please help me out with this:
http://drupal.org/node/163993

Thank you for all your help in advance.

Anonymous’s picture

Status: Fixed » Closed (fixed)