| * bess joins | 01:11 | |
| * bess leaves | 01:36 | |
| * cbeer joins | 07:58 | |
| * rsinger joins | 09:00 | |
| * BillDueber joins | 09:10 | |
| * erikhatcher leaves | 09:22 | |
| * erikhatcher joins | 09:44 | |
| * jaron_ joins | 11:27 | |
| * jaron_ leaves | 11:28 | |
| * erikhatcher leaves | 12:16 | |
| * jkeck joins | 12:22 | |
| * bess joins | 12:46 | |
| * jvenner leaves | 12:53 | |
| * bess leaves | 13:05 | |
| * jvenner joins | 13:13 | |
| * bess joins | ||
| * erikhatcher joins | 13:28 | |
| * ndushay leaves | 13:32 | |
| * erikhatcher leaves | 13:36 | |
| * erikhatcher joins | 13:40 | |
| * erikhatcher leaves | 13:44 | |
| * bess leaves | 14:00 | |
| * bess joins | 14:19 | |
| * bess leaves | 14:21 | |
| * ndushay joins | 14:26 | |
| * bess joins | 14:33 | |
| * bess leaves | 14:39 | |
| * bess joins | 14:46 | |
| * bess leaves | 14:50 | |
| * bess joins | 14:52 | |
| * bess leaves | ||
| * erikhatcher joins | 15:02 | |
| * bess joins | 15:22 | |
| * erikhatcher leaves | 15:28 | |
| * mattmitchell joins | 15:47 | |
| <jrochkind> | mattmitchell! | 15:48 |
| <mattmitchell> | jrochkind: how ya doing? | |
| jrochkind: whew, finally. what a week. | ||
| <jrochkind> | not bad. last day of work for the week, then c4l. that is unfortunate that you can't make it. | 15:49 |
| <mattmitchell> | jrochkind: i'm kinda sad about that. | |
| <jrochkind> | mattmitchell: yeah, what happened there? i've met you there before, no? But anyway, probably don't matter. | |
| so, I'm returning to my nearly-done 'more facet values' code, after looking at bad marc data for a couple weeks instead. A couple things I'd like advice/sanity-check on as I proceed. | ||
| <mattmitchell> | jrochkind: no i met you down here at UVa last year though. | |
| jrochkind: sounds good, you have a repo somewhere? | 15:50 | |
| <jrochkind> | http://github.com/jrochkind/blacklight/tree/more_facets_link | |
| <mattmitchell> | jrochkind: ahh right just found it | |
| <jrochkind> | So should I just get into it? | |
| <mattmitchell> | jrochkind: go for it! | 15:51 |
| <jrochkind> | First thing involves a helper method for clicking on a facet value. Recall that the 'ordinary' sidebar facet panel uses ApplicationHelper#add_facet_params when you click on a facet value. | |
| So, the catalog/facets action needs something similar to call when you click on a facet value. | ||
| <mattmitchell> | jrochkind: right ok | |
| <jrochkind> | But it can't just use the existing add_facet_params, cause it needs a couple extra things. | |
| 1) It needs to insist on redirecting to the "index" action, not the "facet" action. | 15:52 | |
| <mattmitchell> | jrochkind: agreed | |
| <jrochkind> | 2) It needs to strip out ALL facet-action-specific request params before redirecting. (If this isn't clear to you why it needs to do this, I can argue for it, it is clear to me). | |
| and that's it. | ||
| <mattmitchell> | jrochkind: so number 2, you mean like facet offset and start? | 15:53 |
| <jrochkind> | offset, limit, sort. | |
| Also id. | ||
| <mattmitchell> | jrochkind: got it | |
| jrochkind: yeah this is how i thought it worked, but must have broken it since | ||
| <jrochkind> | (I can show you the bad effect of NOT doing that in the live UVa facets code, if you don't believe me on #2. But it's easier if it seems to make sense. :) ) | |
| <mattmitchell> | jrochkind: absolutely, i agree | 15:54 |
| <jrochkind> | Okay, cool. The currently live UVa blacklight actually doesn't strip everything out, just in case it matters, but that's not what we're talking about. So, moving on. | |
| Right now, in my branch, I have a cumbersomely named AppliationHelper#add_facet_limit_from_facet_action that simply calls the existing add_facet_params and then does #1 and #2. | ||
| You can look at that code if you want. | 15:55 | |
| <mattmitchell> | jrochkind: ok let me see... | |
| <jrochkind> | But what I wonder is if it add_facet_params ought to just plain do this itself out of the box, and then we only need one method, which makes certian other stuff more DRY too (the sidebar facets list and the catalog/facet stuff can share more code if they're both using the same helper method). | |
| http://github.com/jrochkind/blacklight/blob/more_facets_link/app/helpers/application_helper.rb | ||
| <mattmitchell> | jrochkind: ok i see. i see the paginator is a little diff too | 15:57 |
| <jrochkind> | or maybe leave add_facet_params the same, it does exactly what it's method says. But add a new method add_facet_params_and_goto_index or something that's used by both sidebar facets and catalog/facet. | |
| That last thing I said is actually what I think I should do, heh. | 15:58 | |
| yeah, paginator is a bit different, still potentially evolving. | ||
| <mattmitchell> | jrochkind: ok thinking... | |
| <jrochkind> | that request_keys method was added pretty much just to make it possible to know what they are and to strip em out, while keeping their specification DRY. | |
| * bess leaves | ||
| <jrochkind> | be right back while you think, back in 2 minutes. | |
| <mattmitchell> | jrochkind: cool | 15:59 |
| jrochkind: i think we have a helper called "link_back_to_search" too, wonder if we could mix/match... need to look at it to know though. | 16:00 | |
| * bess joins | ||
| <mattmitchell> | jrochkind: so you're proposing 2 diff solutions for the "add facet param and redirect" process... | 16:01 |
| jrochkind: 1) modify add_facet_params to strip out stuff (offset, start etc.) or maybe only allow a set of valid params through (f, q, etc.) | 16:02 | |
| jrochkind: 2) add new method to do this specifically for this catalog/facet action? | ||
| <jrochkind> | back. | 16:03 |
| So, actually, during this conversation I came up with a #3. | ||
| #2 is what I have in code now. #1 is what I what I was initially suggesting might be better. | ||
| <mattmitchell> | jrochkind: gotcha | |
| <jrochkind> | But #3 is: leave add_facet_params the same. But add a new method which is used by BOTH sidebar facets and catalog/facet, which calls add_facet_params, strips out bad stuff, and forces redirect to index. | 16:04 |
| So add_facet_params is still there doing just what it does if you need it, but both sidebar _facets and catalog/facet can use common code, to allow them to use more common code on top of that. | ||
| <mattmitchell> | jrochkind: sure sounds good. i like 1 and 2 just fine. | 16:05 |
| jrochkind: woops, i mean 1 and 3 :) | ||
| <jrochkind> | heh, so you like #1 #2 and #3 all equally? :) | |
| Oh, okay #1 or #3. I'll go with #3 then. See, this is why I like talking about things, I hadn't even thought of #3 until I had to explain it to you, heh. | ||
| <mattmitchell> | jrochkind: awesome. makes a lot of sense. | 16:06 |
| <jrochkind> | No idea what the method in #3 should be called, show_results_with_added_facet or something. But anyway. | |
| Okay, couple more somewhat easier questions. | ||
| So, the Paginator I inherited from you has a :limit argument. Recall that there's this n/n+1 thing, where what we ask for from Solr actually one more than we actually plan to display. you with me? | ||
| <mattmitchell> | jrochkind: yep totally | 16:07 |
| <jrochkind> | okay, so the Paginator I inherited from you, the :limit matches the n+1. You tell it how many you asked Solr for, and subtracts one to figure out how many to actually display. | 16:08 |
| That does go with the general principle of solr transparency. | ||
| however. | ||
| There are several other places where a limit needs to appear in various apis/params/etc. A limit can appear in the request params itself, to over-ride in your HTTP request how many to show. | 16:09 | |
| A limit will probably appear _somewhere_ in 'config' (loosely understood, maybe just in a method return) to specify how many results a given sidebar facet should page at. Etc. | ||
| <mattmitchell> | jrochkind: ok | 16:10 |
| <jrochkind> | It seems to me that these all should match. If some are n and some are n+1, that gets confusing. They should all be the same. And if they're all n+1 rather than n.... it's a little bit confusing. To like pass a limit=>11 in a request param, to mean "page 10 at a time". You know? | |
| so despite the agreed upon principle of solr transparency, I kinda think these should all be n -- the actual amount of items that will be shown on a page. Not n+1. | 16:11 | |
| <mattmitchell> | jrochkind: oh it's totally confusing. i struggled with this myself. | |
| jrochkind: but then the n+1 issue with facet values? | ||
| jrochkind: kinda of an exception maybe. | ||
| jrochkind: but that weirdness could still be hidden | 16:12 | |
| <jrochkind> | Well, you still need to ASK for n+1 from solr. But all the Blacklight API is n, is my suggestion. But yeah, the Blacklight methods that actually construct the solr request will know to construct the request with n+1. | |
| <mattmitchell> | jrochkind: right | |
| jrochkind: oh, is it currently n+1 in the url? | ||
| <jrochkind> | After all, maybe in the future Solr gets smarter. We have another way to accomplish this without asking for n+1. Then we can just fix the implementation, but all the 'api', request params etc, can stay the same -- it's specifying the outcome, not the implementation. Is my thinking. | |
| <mattmitchell> | jrochkind: yeah very good +1 (no pun intended) :) | 16:13 |
| <jrochkind> | I think what I inherited, it wasn't in the URL at all. But it's n+1 in the Paginator. I'd like to make it n in the paginator, n everywhere. The paginator will know that the :limit is the number to display, not the number that was requested from solr. | |
| Okay, cool. | ||
| <mattmitchell> | jrochkind: love it | 16:14 |
| <jrochkind> | so, just 1.5 more, heh. | |
| So somewhere in your app, you need to specify which sidebar facets should get a 'more', and likely what the limit is for that specific facet. We know that WHERE you do that is somewhat controversial, heh. Without worrying about exactly where that is for the purpose of this conversation... | ||
| <mattmitchell> | jrochkind: right :) | 16:15 |
| <jrochkind> | Knowing that it's controversial and might change, i want a method that's like facet_limit_for("format_facet") or something, that EVERYTHING that wants to know it calls. So if where it's stored changes, we can just change the implementation of that method, great. Nobody else cares where it is, they just ask that method. | |
| <mattmitchell> | jrochkind: exactly. that was something i did a while ago for all of that Blacklight.config stuff. It was sprinkled in the views, but now helpers abstract the source. | 16:16 |
| <jrochkind> | Additionally, that method should be somehow tied to a controller, NOT in a global singleton like Blacklight. So you can easily have it be different for different controllers. Per some previous conversations we had. Make some sense? | |
| <mattmitchell> | jrochkind: yeah, we're on the same page with this | 16:17 |
| <jrochkind> | s/like Blacklight/like Blacklight.config/ | |
| Right, so, okay.... thing is, it needs to be available from Controller code AND from View/Helper level code. | ||
| So the Rails way to do that, is to put it in a Controller, and just say "helper_method :my_method" to make it avail to views/helpers too. | ||
| <mattmitchell> | jrochkind: ok, so a controller method pushed out as a helper? | |
| jrochkind: right | ||
| <jrochkind> | Right, exactly. | |
| But the trick is, that makes it hard to put it in a module. Either I put it directly in CatalogController... Or I have to use various Rails magic to add it is as a helper method even though it ain't. | 16:18 | |
| <mattmitchell> | jrochkind: you can still put that in a module without any rails magic | |
| <jrochkind> | No way to push it out as a helper if it's in a module, without ruby tricks. | 16:19 |
| Ruby tricks like having the module implement module_added(adding_class), and then calling adding_class.helper_method. | ||
| <mattmitchell> | jrochkind: not a ruby trick though, standard stuff really... let me find an example | |
| <jrochkind> | Can't just put "helper_method :my_method" in a module. | |
| <mattmitchell> | jrochkind: well that's true, but no big deal really? | |
| <jrochkind> | yeah, okay, so seems like the best thing to do is put it in a module, and use the module_added trick? That does not seem unreasonable to me. | 16:20 |
| So the question is... WHAT module? Where should this method go? | ||
| And if it's in a module that's included by a module that's included by the actual CatalogController... the ruby magic gets even thicker. | ||
| <mattmitchell> | jrochkind: yeah this is what i did last week: http://github.com/projectblacklight/blacklight/blob/app_controller_helper_modules/lib/blacklight/app/controller/application.rb | 16:21 |
| <jrochkind> | right on. #included not #module_added, I got my method names wrong. But, yeah, I can do that. | |
| oh wait, is that actually committed to trunk now? | ||
| <mattmitchell> | jrochkind: no, seperate branch | |
| <jrochkind> | Oh, heh. | |
| <mattmitchell> | jrochkind: i'd like to though | 16:22 |
| <jrochkind> | Yeah, you should do it. I support you in that. | |
| <mattmitchell> | jrochkind: right on thanks | |
| <jrochkind> | But meanwhile, right now, for my patch... where should I stick this limit_for_facet(facet_name) config lookup thing? | |
| <mattmitchell> | jrochkind: well, the simplest way would be the CatController | |
| <jrochkind> | PS: You could have that #included check to make sure base.respond_to?(:helper_method) before calling it, in case someone wants to include the module in something that isn't an ActiveController. If that makes sense to do. | 16:23 |
| <mattmitchell> | jrochkind: but it would be nice to start organizing some of our gelpers into modules | |
| <jrochkind> | Yeah, I have no idea, esp with this stuff in transition with your branch. Basically, just tell me where you think ti's best (for now, for my patch, with your branch not yet committed), and I'll do it that way, heh. | |
| <mattmitchell> | gelpers, hmm i mean helpers | |
| jrochkind: so we're talking one helper method, maybe just the controller for now, and then call helper_method? | 16:24 | |
| <jrochkind> | mattmitchell: Yeah, okay, since this stuff is transition anyway, I'll just put it in CatalogController for now? With a big comment saying it's config-lookup kind of stuff that needs to be moved somewehre else at some point. Good for now? | |
| I _think_ it's just one method, yeah. Maybe by the time I'm done it'll be two. but one or two. | 16:25 | |
| <mattmitchell> | jrochkind: yeah i think that's perfect | |
| <jrochkind> | okay, cool. ONE last question, I think it's even easier. :) | |
| <mattmitchell> | jrochkind: np! | |
| <jrochkind> | So, at first I was assuming that you needed to tell Blacklight, for a sidebar facet, YES 'more values' link this one, and display in the initial sidebar at Limit N. | |
| erik brought up that the code COULD just get the paging from Solr response. Like, you could tell Blacklight, YES, 'more' this sidebar facet, but not have to tell BL the limit. BL would jsut assume the limit is whatever the Solr response's request asked for, minus 1 of course. | 16:26 | |
| I think I still like my way of doing it. But my code COULD accomodate both ways of doing it, at the local implementers choice. | ||
| You follow? If so, any opinion? | 16:27 | |
| <mattmitchell> | jrochkind: hmm, let me re-read eriks way | |
| <jrochkind> | I can explain it more clearly too if you like. | |
| <mattmitchell> | jrochkind: ok yeah | |
| <jrochkind> | it's sort of erik's way interpreted by me, erik didn't understand the actual code, but I think it's erik's way interpreted by what I understand as the constraints. | 16:28 |
| So, this erik/jrochkind way... | ||
| You could just tell Blacklight, like show_more_link_for_facet("format"). | ||
| <mattmitchell> | jrochkind: you mean without caring about whether or not there are actually more facets? | 16:29 |
| <jrochkind> | And Blacklight would look at the Solr Response it was displaying. Look in it for the echoed request params. Look at those echoed request params for a f.format.facet.limit or facet.limit... and know that the amount to show on the page is one less than that. | |
| <mattmitchell> | jrochkind: oh i see | |
| <jrochkind> | Nope, BL will have to calculate if there are more either way. And the way it has to calculate that is by knowing how many you asked solr for. | |
| The question is just whether you tell it the limit in Rails, and then Rails asks Solr for n+1 itself. Or whether you just set the limit in yoru solrconfig.xml, and Rails looks for it in echoed request params. | 16:30 | |
| <mattmitchell> | jrochkind: i see. i don't know really what would be best? | |
| <jrochkind> | Or if the code needs to support both ways of doing it. | |
| I kinda like my way of doing it, but I prefer setting things in Blacklight rather than Solr. I know others prefer different. | ||
| <mattmitchell> | jrochkind: right, i kinda like setting things in BL as well, but see the value in both. | |
| <jrochkind> | To support the Solr way of doing it, some api needs to be changed -- like you'd have to give the Paginator the whole Solr response, so the Paginator could look up the echo'ed request params. Right now the Paginator doesn't have access to that. | 16:31 |
| <mattmitchell> | jrochkind: true, or at least the required values | |
| <jrochkind> | So, yeah, I'm not going to give up on my way. Question is just your advice on, for now, should I spend time making BOTH ways work, or is it okay if at first I just support my way? :) | |
| * bess leaves | 16:33 | |
| <mattmitchell> | jrochkind: i'd just go for the one way, which is your way :) | |
| <jrochkind> | okay, good enough. | |
| If people really want the other way, I can always refactor and add it later. | ||
| <mattmitchell> | jrochkind: unless it makes the design more flexible as a side effect, and it's easy to do... but up to you. | |
| jrochkind: cool | ||
| <jrochkind> | It's not easy to do it, it's kind of a pain to do, otherwise i"d just do it. :) | |
| <mattmitchell> | jrochkind: well there you go :) | 16:34 |
| <jrochkind> | So I think tha'ts all for now! Thanks for letting me bounce it off you, very helpful. Want to make sure I'm doing things that will fit into the overall plan and such. | |
| If it wasn't for code4lib next week, I'd say I'd have a reviewable patch to add 'more' in days. So more like one week plus days, I guess. | ||
| <mattmitchell> | jrochkind: awesome! thanks for doing this, really nice patch to bring in! | |
| <jrochkind> | It seems to be an in-demand feature, someone else was asking for it in channel yesterday. | |
| And, FWIW, I _did_ consider trying to make it an external plug-in. But it gets enough into the guts of Blacklight AND is so much in-demand, that I think it makes sense just to have it in core. | 16:35 | |
| <mattmitchell> | jrochkind: excellent. i'll see if i can get my controller/helper module stuff committed too. | |
| <jrochkind> | mattmitchell: of course, one way or another mine and yours will have to be merged, since I'm adding something to CatalogController. We'll deal with that at the right point, I guess. | |
| <mattmitchell> | jrochkind: right, should't be too difficult and i'd be happy to untangle it! | 16:36 |
| <jrochkind> | I actually made a buncha edits to CatalogerController even without that config-lookup-helper-method issue, because that's where catalog/facet is defined, and I had to fix up that action a bit. | |
| yeah, it'll be do-able. hooray for git. | ||
| <mattmitchell> | jrochkind: no doubt, love git. | |
| <jrochkind> | Oh, and as a warning, in order to make the sidebar facets stuff simple.... I made the Paginator know that a nil limit meant "don't page, just show all of them". So the sidebar can use the Paginator with or without a limit, to not require a giant if/else depending on whether 'more' was desired for that facet. | 16:37 |
| makes sense to me, I have no doubts about it, but might look weird at first. | ||
| oh, and lastly, my FIRST attempt will be WITHOUT any ajax at all. I plan to get that reviewed and committed, and THEN add AJAX on top. Unobtrusive JS! | 16:40 | |
| <mattmitchell> | jrochkind: good deal. i always do thing like that too. | 16:43 |
| <jrochkind> | Yeah, I feel proud when my interface works reasonably well even if browser has JS turned off. :) | 16:44 |
| a few things in BL don't that some day I might get time to try and improve. | ||
| anyhow, enough, thanks for your time! | 16:45 | |
| <MrDys> | pyrrhic_victories+- | |
| <jrochkind> | MrDys: uh oh. what happened? | |
| <MrDys> | you wanting to fix everything to work without javascript | |
| <mattmitchell> | jrochkind: thanks again for this, great work. | |
| <jrochkind> | MrDys: Oh right, I recall now that you've always found that a silly mission. oh well. | |
| <MrDys> | noble, but yes, rather silly | 16:46 |
| <jrochkind> | If nothing else, it makes it increases the potential customer base. The California universities can't use any software unless it works without JS. | |
| It's actually not that hard to do. And I don't think it's silly --- it's not just for blind people, it's for access on shitty mobile browsers, for spider and other machine access, etc. | ||
| Also just a good design principle, in that it makes it easy to, say, swap out Prototype for JQuery later or something, since it forces you to completely seperate the JS from the DOM. | 16:48 | |
| <MrDys> | sure, all good points | |
| <jrochkind> | but I guess in the end, it also just warms my heart. :) | |
| <MrDys> | you're a weird guy, you know | |
| <jrochkind> | indeed. | |
| * bess joins | 16:49 | |
| * jamieorc joins | 16:53 | |
| * mattmitchell leaves | ||
| * ndushay leaves | 16:55 | |
| * jkeck leaves | 17:01 | |
| * jkeck joins | 17:04 | |
| * bess leaves | 17:19 | |
| * jvenner leaves | 17:21 | |
| * bess joins | 17:25 | |
| * jvenner joins | 17:42 | |
| * erikhatcher joins | 18:12 | |
| * erikhatcher leaves | 18:19 | |
| * jkeck leaves | 18:28 | |
| * jrochkind leaves | 18:31 | |
| * jkeck joins | 18:32 | |
| * bess leaves | 19:06 | |
| * Naomi joins | 19:37 | |
| * Guest608 leaves | 19:53 | |
| * ndushay joins | ||
| * erikhatcher joins | 20:03 | |
| * tachyonwill_ joins | 20:10 | |
| * tachyonwill_ leaves | 20:11 | |
| * rsinger leaves | 20:25 | |
| * erikhatcher leaves | 20:45 | |
| * jkeck leaves | 21:17 | |
| * jamieorc leaves | 21:32 | |
| * rsinger_ joins | 21:51 | |
| * rsinger_ leaves | 22:06 | |
| * rsinger joins | 22:19 | |
| * ndushay leaves | 23:15 | |
| * bess joins | 00:09 | |
Generated by Sualtam