The "Need more options" functionality should work as it does for poll modul in core. As for now the checkbox is still checked after a preveiw, and it's not possible to have more options by rechecking it.
Also, when a decision has been created with only one option the functionality is completely broken. This is probably solved with http://drupal.org/node/86659
And a bit unrelated. This functionality could of course be enhanced with javascript adding the options with no page refresh. This shouldn't remove the current functionality though as this is important for non-javavscript users (in other words: use progressive enhancement).
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | decisions.module_morechoices7.patch | 11.81 KB | ChrisKennedy |
| #16 | decisions.module_morechoices6.patch | 11.1 KB | ChrisKennedy |
| #15 | decisions.module_morechoices5.patch | 9.19 KB | anders.fajerson |
| #12 | decisions.module_morechoices4.patch | 9.05 KB | anders.fajerson |
| #11 | decisions.module_morechoices3.patch | 8.78 KB | anders.fajerson |
Comments
Comment #1
ChrisKennedy commentedI fixed the "stays checked" issue on the 9th with http://drupal.org/cvs?commit=42173 but I forgot that there was an issue on it (I had actually just been browsing issues for poll.module and saw that decisions made the same mistake). Still investigating why there is a 10-option limit.
Yeah I need to start trying some JQuery stuff for this and the ranking ballot.
Comment #2
anders.fajerson commentedFirst make it work server side, then jQuery, OK :)
I also intend to play some with jQuery, but I'm waiting for the 5.0 release ;)
Comment #3
ChrisKennedy commentedThis took forever to fix, but I think I tested all the cases. There are a lot of combinations though so there could be bugs. http://drupal.org/cvs?commit=42629
Comment #4
anders.fajerson commentedThe behaviour we're after is the same as for poll.module, right? Just so I know what to expect :)
This is weird, but it works form me in Firefox but not in Opera 9.02. After selecting "need more.." the second time, the 10 choices are back to 5 and the filled in form is completely empty.
Just glancing at the code I see that it differ quite a lot from how poll.module handles this (4.7 and 5.0 does it slightly different. 5.0 recently fixed a bug related to this). Why is that?
Comment #5
ChrisKennedy commentedIt doesn't necessarily have to be the same as poll.module, since it isn't a "best practice" module and "more choices" has only recently been somewhat fixed in it.
For example, I made it so that when previewing a decision and setting less than 5 choices it won't show the extra blank ones, whereas poll.module does (erroneously in my opinion). I designed the functionality around what would make the most sense theoretically - so it may or may not differ from poll.module's UI.
The code differs because the functionality is a bit more complex and I wanted to explicitly determine which path we were in. It might be possible to consolidate some of the code but an expanded form can be clearer to edit. I tried to use the code from the 5.0 version of poll.module but it didn't work for me at least.
In general the code will only be the same where we have copied code from poll.module and haven't yet customized it - over time it will inevitably diverge.
Comment #6
ChrisKennedy commentedVery strange on Opera - it isn't displaying the ChoiceGroup parent array on any of the values for some reason. I have no idea why this is occuring on Opera. It could be a bug in decisions, the form api, or Opera.
Comment #7
anders.fajerson commentedHere's a complet rewrite, going back to poll.module's way of generating the "more choices". It doesn't fix the Opera bug, but I've commented out the part causing the bug. Chris, could you have a look at it again?
Comment #8
anders.fajerson commentedKeeping up with the latest changes.
Comment #9
ChrisKennedy commentedAny implementation of morechoices should have the following functionality, which my patch implements, unless we decide on something better:
1. If morechoices was checked, show twice the number of previously shown choices (including blanks)
2a. Otherwise show the number of non-blank choices.
2b. But if there were no non-blank choices defined, default to 5.
Poll.module messes up the preview of a new poll by showing 5 choices even if fewer choices were specified (i.e. the form doesn't accurately reflect the preview in my opinion).
And again, significant reversions to poll.module code without a good reason is problematic:
1. poll.module is consistently mentioned as the worst module in core and the developers recognize that it is buggy and poorly maintained. See, e.g. http://drupal.org/node/61285
2. It is likely to introduce various bugs that will take even more time to track and fix, especially given the magnitude of the code change.
3. It ignores the reasons that certain code was changed in the first place. For example, you reverted the fix from http://drupal.org/cvs?commit=42173 Your patch assumes that all differences between the two implementations are inconsequential, which isn't the case.
4. It prioritizes an unlikely case (Opera user) over a likely case (previewing).
If it doesn't fix the Opera bug, reduces functionality, reverts an unknown number of previous bugfixes, and introduces the potential for integration bugs, it simply isn't an appealing patch.
Comment #10
anders.fajerson commented1, 2a, 2b: I never understood this feature, that's why it was overwritten in this patch. Now that I understand it I think it's an awkward behaviour and something we not really need to do. What's the user case?
1: From what I understand this issues mainly complain about not using VotingAPI and Views, which we do or plan to do. It says nothing about the actual code quality. I still think this module and the rest of core is what we should mimic in terms of using the Drupal APIs, and not role are one solutions (e.g. by using a lots of $_POST['edit'])
2: This isn't relevant. That's why we review patches before they go in (or at east my patches ;) )
3: I think you are wrong here. That fix applies to Drupal 5.0, not 4.7.
4: I use Opera :) And to be fair I asked if you could take a look at the "preview code", so this path was (and is) a work in progress. I will file a bug about Opera, which is an other issue.
If it doesn't fix the Opera bug, reduces functionality, reverts an unknown number of previous bug fixes, and introduces the potential for integration bugs, it simply isn't an appealing patch.
You're missing the main point (and I might have been unclear on that), that the code it replaces IMHO is really unclean and an nonstandard way of using the formAPI. This code is much easier to understand in many ways (just by using $choice['chtext'] instead of $choice we lower the risk of missing check_plain, which by the way we had and is added with this patch). It's also a step closer to how poll.module 5.0 is doing it, which is even better. If we can agree on that this code is cleaner in general, let's fix the features it's missing. And let's review it carefully to try to avoid introducing bugs.
I rerolled the patch. This time I didn't comment out the Preview code, which still needs to be updated to work with this code change.
Comment #11
anders.fajerson commented..and the patch.
Comment #12
anders.fajerson commentedNew patch to work with the new votes tab
Comment #13
ChrisKennedy commentedAlright you convinced me :)
You might as well comment out the preview code. The only thing that needs to be fixed is to not show blank choices in the preview.
The diff should have -F^f so that we can see which function the patch block is in.
Decisions is spelled "Desicions" in one _form() change.
Did you mean to take the collapsible=TRUE out of the choices fieldset?
Comment #14
anders.fajerson commentedI don't mind having blank choice field after a preview. I wouldn't expect the actual form to change after a preview, just that a preview of the output of the form is viewed, which is the current behaviour.
If there are blank choices it most often means that the user has bothered to check the need more choices and hit preview once before, and I don't think we need to clean up after that. What's confusing about having those blank field? As we give the user 5 choices to start from, and no possibility to delete superfluous choices in the UI, it's quite obvious that if you leave them blank it means you just wan't less than 5 (or less than 10, 20, 40 etc), and that is also confirmed in the preview.
I've fixed the small issues in this new patch. I haven't done anything to the Preview code since it's over my head. Also, I noticed that this patch made it impossible to vote in the first choice in a poll. To sum up:
1. Fix voting on first choice
2. Fix preview (revert to old behaviour)
3. Decide how to do with blank votes (my suggestion is to skip that and focus on the other two, since this patch needs to get in)
Comment #15
anders.fajerson commented..and patch.
Comment #16
ChrisKennedy commentedOkay this patch removes the old preview code and lets you vote for the first choice.
I think that eventually we shouldn't show blank choices in the preview or form, but yeah, we should commit this first.
Comment #17
ChrisKennedy commentedOkay I committed this patch because it's a lot of changes and maintaining diffs for the other issues I want to work on would be too much of a pain.
Comment #18
anders.fajerson commentedNow I'm a bit confused. You were talking about the blank choice text fields in the actual form before, right? Well, I was. With this patch blank choice checkboxes are shown in the vote preview... New battle plan ("back to square 1"):
1. Go back to poll.module's behaviour and render the result, this will also fix Opera, http://drupal.org/node/90102
2. Open a new issue (I couldn't find one for when we implemented the vote form preview) for previewing vote form instead of the result. I would guess that such a solution would use a "dummy" rendering of the form.
Anyaway, I'm marking this as fixed. Hopefully we have at least learnt something from this hideous issue - I have. Thanks for your work Chris.
Comment #19
(not verified) commented