Richard Heck wrote:
> Attached is a patch fixing bug 1550. It pre-empts my previous effort,
> using radio buttons, as suggested by Abdel and JMarc, among others,
> rather than a combo box, as previously.
Very good work Richard! Thanks for doing this.
>
> This patch makes the dialog modal, for the present. This is not the
> ideal solution, but the ideal solution is still not known, the issue
> being that, if left up, the dialog does not get updated if the user
> moves the cursor with the keyboard or mouse and hence may still offer
> invalid options, as well as (say) presenting a disabled Label Width
> field even though the cursor is in an itemized list.
OK, I know what the problem is and I agree with you that this is more or less a
design problem. But we can work around this problem; we have two solution:
1) refresh the dialog contents in QParagraph::update_contents(). We just need to
create a helper function in ControlParagraph that will return the current
paragraph attributes. I know this looks weird but we don't have much choice.
2) cleanup the dialog by truly separating the model/controller from the view as
was done for the TOC and Citation dialog. If we do that, QParagraph will be
_the_ controller and the dialog update will be done in
QParagraphDialog::update().
I propose that you commit your patch (taking into account my comments below)
with the modal dialog. Then, I'll help you fixing this update issue.
>
> One question: I would have preferred to make QParagraph::radioMap const
> but couldn't, because it can only be populated after the dialog is
> created. Any other ideas?
I think we don't really need this QPRadioMap. We have only 4 radio buttons, why
don't you access them directly instead?
Some more style comments below:
> +void QParagraphDialog::checkAlignmentRadioButtons() {
> + if (alignDefaultCB->isChecked()) {
> + for (
> + QPRadioMap::iterator it = form_->radioMap.begin();
> + it != form_->radioMap.end();
> + ++it)
I think the preferred style in LyX is:
+ QPRadioMap::iterator it = form_->radioMap.begin();
+ QPRadioMap::iterator end = form_->radioMap.end();
+ for (; it != end; ++it)
> + {
> + it->second->setDisabled(true);
> + }
> + } else {
> + LyXAlignment alignPossible =
> form_->controller().alignPossible();
> + for (
> + QPRadioMap::iterator it = form_->radioMap.begin();
> + it != form_->radioMap.end();
> + ++it)
ditto.
> +LyXAlignment QParagraph::getAlignmentFromDialog()
> +{
> + if (dialog_->alignDefaultCB->isChecked())
> + return LYX_ALIGN_LAYOUT;
> + LyXAlignment alignment = LYX_ALIGN_NONE;
> + for (
> + QPRadioMap::iterator it = radioMap.begin();
> + it != radioMap.end();
> + ++it
> + )
ditto.
> Index: qt4/QParagraph.h
> ===================================================================
> + ///
> + LyXAlignment getAlignmentFromDialog();
> + ///
> + void alignmentToRadioButtons(LyXAlignment align = LYX_ALIGN_LAYOUT);
> + ///
> + QPRadioMap radioMap;
As I said above, I am not sure this QPRadioMap is needed at all. Maybe it's
better to just use the radio buttons directly?
> Index: controllers/ControlParagraph.C
> ===================================================================
> +
> +string ControlParagraph::lyxAlignmentAsString(LyXAlignment const align)
This method is not needed nor used (except in a debug statement). I think you
can ditch it.
Abdel.
