Bug 27772 - PopupMenuQt lacks disabled and selected support
Summary: PopupMenuQt lacks disabled and selected support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 23712 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-07-28 11:31 PDT by Mike Fenton
Modified: 2009-11-24 06:39 PST (History)
2 users (show)

See Also:


Attachments
Patch that enables selected and disabled support for PopupMenuQt. (1.63 KB, patch)
2009-07-28 12:05 PDT, Mike Fenton
manyoso: review-
Details | Formatted Diff | Diff
Patch with updated style / model check. (1.63 KB, patch)
2009-07-30 11:37 PDT, Mike Fenton
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2009-07-28 11:31:48 PDT
PopupMenuQt previously had a code block defined out that was meant to provide the ability to disable entries and select the current entry.

This functionality should be re-instated.
Comment 1 Mike Fenton 2009-07-28 12:05:30 PDT
Created attachment 33658 [details]
Patch that enables selected and disabled support for PopupMenuQt.
Comment 2 Adam Treat 2009-07-30 09:41:06 PDT
Comment on attachment 33658 [details]
Patch that enables selected and disabled support for PopupMenuQt.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +    QStandardItemModel *model = qobject_cast<QStandardItemModel*>(m_popup->model());
> +    Q_ASSERT(model);

Style decoration problem.  See coding guidelines.

> +            if (client()->itemIsSelected(i))
> +                m_popup->setCurrentIndex(i);

The big fear I have is that QComboBox uses QStandardItemModel internally... if they change that to using something else we'll be asserting.

How about this... Still ASSERT on the model, but further down replace with:

    if (model && !client()->itemIsEnabled())

That way we won't worry in release builds.

Sound ok?
Comment 3 Mike Fenton 2009-07-30 11:37:53 PDT
Created attachment 33795 [details]
Patch with updated style / model check.

Thanks for the review.   Patch is updated as suggested.
Comment 4 Adam Treat 2009-07-30 11:58:27 PDT
Comment on attachment 33795 [details]
Patch with updated style / model check.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +
> +            if (model && !client()->itemIsEnabled(i))
> +              model->item(i)->setEnabled(false);
> +
> +            if (client()->itemIsSelected(i))
> +                m_popup->setCurrentIndex(i);
> +        }

Indentation is not quite right, but I'll fix it when I land.  Otherwise r=me!
Comment 5 Adam Treat 2009-07-30 12:08:14 PDT
Landed with r46593.
Comment 6 Simon Hausmann 2009-11-24 06:39:38 PST
*** Bug 23712 has been marked as a duplicate of this bug. ***