Closed (fixed)
Project:
Node Relativity
Version:
master
Component:
User interface
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
25 Mar 2007 at 10:21 UTC
Updated:
8 Sep 2007 at 19:21 UTC
Jump to comment: Most recent file
Comments
Comment #1
JohnG-1 commentedReview:
This patch to relativity.module creates new Rendering option for children nodes of type ... , to display children as a (views.module) View - it even gives you a list of your views to choose from.
Overall :
<gold> ***** </gold>Some tips on building the view:
If you set the view Arguments thus, you build a single generic view that you can use for displaying all your child nodes each in their respective child_type_table (one-size-fits-all :).
This shows only children of the current node.
This allows a view Filter [Node: Type] to select which child_type_tables are displayed / hidden. (returns the view [Empty text] for hidden child_types)
This shows the children in the correct child_type_tables.
Page Views work fine but I have not managed to get everything working in a Block View yet (still arguing with the Argument Handling Code ...).
Comment #2
darius commentedVery nice work!
Three issues before I apply this patch:
1) There's been some minor code changes in CVS HEAD, so the patch does not apply cleanly anymore.
2) Please use 2 spaces, not tabs for indentation (see Drupal coding style guide)
3) Quoting:
-0.1 Quibble: the method for hiding the Link Operations does not appear degrade properly: 'Remove', 'Create ...' and 'Attach ... ' links are not available if the display option is not a view. (Like I care! ;)
This is very important! We should care about graceful upgrades. For a lot of people using views for displaying a few children is an overkill. Backwards compatibility is a must here. If views are not installed or used, this patch would cripple existing functionality. This change should not be too hard to implement, though.
4) I would prefer the views related code to be implemented in a separate function (maybe move to relativity_views?). The theme function is already too cluttered as it is.
Thanks for all the good work.
Comment #3
darius commentedVery nice work!
Three issues before I apply this patch:
1) There's been some minor code changes in CVS HEAD, so the patch does not apply cleanly anymore.
2) Please use 2 spaces, not tabs for indentation (see Drupal coding style guide)
3) Quoting:
-0.1 Quibble: the method for hiding the Link Operations does not appear degrade properly: 'Remove', 'Create ...' and 'Attach ... ' links are not available if the display option is not a view. (Like I care! ;)
This is very important! We should care about graceful upgrades. For a lot of people using views for displaying a few children is an overkill. Backwards compatibility is a must here. If views are not installed or used, this patch would cripple existing functionality. This change should not be too hard to implement, though.
4) I would prefer the views related code to be implemented in a separate function (maybe move to relativity_views?). The theme function is already too cluttered as it is.
Thanks for all the good work.
Comment #4
JohnG-1 commentedI'm attaching a patched (relativity_4_0.patch) copy of the whole relativity.module so people can test it without going through the rigmarole of applying the patch themselves ...
@ darius 3 - 'Quibble' - yes of course you are right this does need to be addressed for non-views users. On the point about view tables being overkill for lists of one or two children I also agree, but the view can be set to return a list format (or teaser, or full node even) instead of a table ... views gives you more list theme/configuration flexibility than Relativity could realistically ever provide.
@ darius 4 - 'views_relativity.inc' - +1 this idea: it would get around problem 3 ... is it just a matter of moving the added code across or would the views stuff need to be rebuilt in to separate functions to be placed in the views.inc ? what happens if you have 2 copies of "function relativity_links" ...?
Comment #5
JohnG-1 commentedMinor bug appears when relativity_4_0.patch is applied.
I guess this means that some rendering option variable is not being reset properly when the Allowable Child Node Types is modified ... I have NOT tested this with an unpatched version of Relativity, but I think I would have noticed it before if the bug is not brought to light by the patch.
Sorry, Omar :(
Comment #6
JohnG-1 commentedI would like to see this patch go forward but as a non-programmer I'm a little bit intimidated by some of the things that need doing. Anyway here's my thoughts on each change that Omar's patch makes. Some don't seem necessary, others would need someone cleverer than me to get them working from the relativity_views.inc file. Caveat: Please bear in mind that I have no idea what I'm talking about ;)
is safely wrapped in "if(module_exists(views)) ..." ?
is now unnecessary because darius' p.patch allows the Link Operations to be disabled from the Display Settings ?
the big addition to theme_relativity_links function. I don't know how this can be moved into a discreet function so it can go into _views.inc. I suspect that the theme functions would benefit from being broken down into a series of smaller functions in order to allow _views.inc functions to hook in. I need help with how to do this ?
I think is part of the above task. The new function relativity_handle_view_links should move to _views.inc easily ?
only seems to call drupal_get_destination() to return the user to the parent node after creating a new child ... I don't see why this is added ?
Would anyone else be interested in having a look at this?
Comment #7
darius commentedJohn, thanks for your efforts. I will get to this very soon.
Comment #8
owahab commentedJohnG: thanks for your continous efforts in relativity module.
@@ -633,6 +633,19 @@
is safely wrapped in "if(module_exists(views)) ..." ?
You're probably right, it should be done that way but let me think out loud: this will only be necessary if the user chosen a view to render children type foo (this action requires views to be enabled otherwise the user won't see the list of views) then after hitting save, the user disabled views module.
I don't know but I feel this will give the user a hint why the view isn't rendered.
Probably the best practict is to have a "else" statement to handle this case.
@@ -1104,12 +1117,14 @@
is now unnecessary because darius' p.patch allows the Link Operations to be disabled from the Display Settings ?
That's right.
@@ -1272,6 +1287,64 @@
the big addition to theme_relativity_links function. I don't know how this can be moved into a discreet function so it can go into _views.inc. I suspect that the theme functions would benefit from being broken down into a series of smaller functions in order to allow _views.inc functions to hook in. I need help with how to do this ?
Well, the function actually is doing a foreach and i didn't see a better way to do the rendering away from the foreach.
Will re-think about this.
@@ -1293,19 +1366,26 @@
I think is part of the above task. The new function relativity_handle_view_links should move to _views.inc easily ?
True.
@@ -1390,7 +1470,7 @@
only seems to call drupal_get_destination() to return the user to the parent node after creating a new child ... I don't see why this is added ?
This should have gone in a separate patch: why not send the user to the parent node when the follwing use case happens:
1. The user clicks "attach ..." link.
2. Fills the new node from.
3. Click submit.
What happens now is the user going to the node view screen which didn't seem to be reasonable.
Comment #9
jastraat commentedThis is exactly what I was looking for! What is the status on this patch?
Comment #10
owahab commentedComment #11
goose2000 commentedGot the full patched mod today, will test. Want to say so far, this is the
best, easiest to understand and use parent-child node builder I have
come into. Nice work.
Comment #12
goose2000 commentedWill this become part of relativity.module ? I guess it didn't make it in the last release.
Comment #13
darius commentedOK, this important issue needed some restarting, so here we go.
I took Omar's patch, updated it, cleaned it up, and applied to 5.x-2.0-dev (you might have to wait for 24 hours before it gets packaged automatically for download). The CVS tag is DRUPAL-5--2. I am attaching the updated patch, too, so you can also just apply it to official release 5.2.
This patch needs some serious testing and reviewing. It might not work at all yet, since I did not do much testing. So, if anyone can help testing, please do so. If you the review is long and detailed, maybe it is even better to open a new issue.
Thanks.
Comment #14
goose2000 commentedHi, I started with the patch this AM, Good and Bad:
Good:
patch applied fine.
the option to use views did appear in the select box
It showed the view, with some problems.
Bad:
Errors, but thinking the better way to test is to setup a simple and new
test case before I go pasting errors all over. I'll set that up now...
IIS 5, PHP 5.1, MySQL 5.0, Drupal 5.1
John A
Comment #15
goose2000 commentedHi, I set up a fresh parent - child relation between to new CCK nodes, with the child having a few custom text fields added.
Made a view for the child. It included Title, and the 3 custom text fields (name, organization, FTE). Tested the view to see it work on its own, all fine. Then went and applied this to my display options in the admin relativity module, for the parent to display the child.
When I load the page I get some errors:
# warning: Invalid argument supplied for foreach() in C:\Inetpub\wwwroot\drupal\modules\node\node.module on line 504.
Hmmm# warning: implode() [function.implode]: Bad arguments. in C:\Inetpub\wwwroot\drupal\modules\node\node.module on line 508.
# user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT n.nid, n.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.sticky, r.timestamp AS revision_timestamp, r.title, r.body, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM node n INNER JOIN users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE in C:\Inetpub\wwwroot\drupal\includes\database.mysql.inc on line 172.
And below this, you can see the view, table format. But it shows it 3 times. So there were only 3 child nodes that would show, but it (the view table) was shown 3 times. So If I add another, I expect there will be 4 tables shown.
Loop issue, hmm. - Thanks for continuing this - John
Comment #16
goose2000 commentedOne other good note; after switching back to show 'just title' all functions appear to be working normal. Degrades well.
Comment #17
darius commentedA new patch. Cleaned up some more code, seems to work better now. To get rid of the warnings, I commented out the following field from the views function -- can someone tell me what it does (it's Omar's code)?
Also, could someone test this new patch with two different children types, maybe setting one type to display as a view, and another - as simply an old list of titles?
Comment #18
owahab commenteddarius,
First I'm sorry for being away the past time.
Second, this chunk of code adds "Attach/Remove" column to the rendered view.
I will try to test the new patch.
Comment #19
goose2000 commentedI have been using this from CVS:
1.43 5 days ago darius Fixing views patch (issue 130870)
It is working good! I can have either views or title lists.
Comment #20
kirilius commentedAny idea when the patch will become part of the release?
Comment #21
darius commentedYou can download it at http://ftp.osuosl.org/pub/drupal/files/projects/relativity-5.x-2.x-dev.t...
Once a few people say it works, I might add an "official" release with this patch.
Comment #22
kirilius commentedI tried it (downloaded from the above link) and it did not work for me. I have a few questions:
- Is the view specification from the initial post mandatory? I mean do I always need 3 parameters? I just need to understand why do I need them. What is going to happen if I leave only the first one?
- The initial post said that there was a problem with the block view. Is that still true?
What I am trying to achieve:
1) I have an A-type node with multiple child nodes of type B-node
2) B-node is an image (created using CCK, imagefield and Imagecache)
3) In the full view of the A-node I want to see it's contents followed by a couple of image thumbnails of the B-node children + a link "more"
4) That link will lead to a page where all B-node children are listed along with the link operations (remove, etc.)
1 and 2 are fine
3 and 4 will be easily implemented with a combination of a Block and Page View. So I created the view, told Drupal to create both block and page, checked "More link", added a thumbnail field, then added the tree arguments as described in the initial post and... I see nothing under the A-node contents?
Is this because of the block-view support is still missing or I am doing something wrong?
In fact I already achieved something very close to that using the advice given here: http://drupal.org/node/151412
The part that I couldn't do using that method was the built-in link operations in the view + some other minor details. Besides the solution added in this patch is much more generic and elegant.
Any idea when the block-view support will be added?
Thank you!
Comment #23
owahab commentedFirst: the patch sends three parameters to the called view which does not imply that you "should" have three parameters defined in your view.
Second: I think what you're trying to do can not be achieved without views but you will still need to tweak the view a little bit.
So you can create a view as mentioned in http://drupal.org/node/151412 and then use the -dev relativity to show that view below nodes of type A.
I have tested that and it worked.
Comment #24
kirilius commentedSo that view will work in a block?
Comment #25
kirilius commentedOne more question. In the initial post you write:
"+1 : It generates a column of 'Remove' links in the table, appends the 'Create new ...' and 'Attach existing ...' links to the view footer (replacing the Link Operations of old)."
Where are these links added? Which "table" is that? Is that the table from the custom view output? If this is true, what types of views are supported? Will the bonus pack grid view work?
I am kinda lost here... ;-)
Thanks for your help!
Comment #26
darius commentedThese features were removed from the patch. We have to iron out the existing views functionality first. If the features are needed, open a new issue, please.
Comment #27
kirilius commentedThanks, I will!
Comment #28
kirilius commentedIs there any schedule for releasing this patch?
Comment #29
kirilius commentedSo is there any plan?
Comment #30
darius commentedThe patch is released (get the 2.x-dev version), but the branch is not tagged as official release yet. You can still use it, though. It's all quite arbitrary on the module level anyway.
In general, things are slow because I am away till September, and even then I'll be quite busy till October. Unless Omar takes charge in my absence, there will not be much new developments till October.
Comment #31
kirilius commentedThanks a lot, I am really looking forward to the stable version!
Comment #32
owahab commentedComment #33
(not verified) commented