Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Search combo property item with QVariant type #6788

Merged
merged 1 commit into from
Jul 28, 2022

Conversation

norihiro
Copy link
Contributor

@norihiro norihiro commented Jul 23, 2022

Description

When finding current item for a combobox on properties view, use QVariant type to hold current values instead of converting all types into string.

Motivation and Context

A combobox created with OBS_COMBO_FORMAT_FLOAT cannot show the current item. When a modified callback for another item on the properties view returns true, the combobox is initialized again and the selection goes to the top item even though another item is selected.

When adding items into the combobox, userData is generated in the code below.

static void AddComboItem(QComboBox *combo, obs_property_t *prop,
obs_combo_format format, size_t idx)
{
const char *name = obs_property_list_item_name(prop, idx);
QVariant var;
if (format == OBS_COMBO_FORMAT_INT) {
long long val = obs_property_list_item_int(prop, idx);
var = QVariant::fromValue<long long>(val);
} else if (format == OBS_COMBO_FORMAT_FLOAT) {
double val = obs_property_list_item_float(prop, idx);
var = QVariant::fromValue<double>(val);
} else if (format == OBS_COMBO_FORMAT_STRING) {
var = QByteArray(obs_property_list_item_string(prop, idx));
}
combo->addItem(QT_UTF8(name), var);

OBS's type userData generation function
OBS_COMBO_FORMAT_INT QVariant::fromValue<long long>(long long)
OBS_COMBO_FORMAT_FLOAT QVariant::fromValue<double>(double)
OBS_COMBO_FORMAT_STRING QByteArray(const char *)

I'm implementing a plugin on norihiro/obs-color-monitor#54 and found this bug.

How Has This Been Tested?

OS: Fedora 35
Qt version: 5.15.2-31.fc35 and 6.2.3-2.fc35 (I checked with both versions.)

Checked these steps for xshm_input (using OBS_COMBO_FORMAT_INT) and slideshow (using OBS_COMBO_FORMAT_STRING).

  • Create a source.
  • Open it's properties view.
  • Choose another item in combobox.
  • Close the properties view and open it again.
  • Check the combobox shows the same item.
    • Before this bugfix, the top item is always selected at this step for the combobox with OBS_COMBO_FORMAT_FLOAT.

Also checked another source under development using OBS_COMBO_FORMAT_FLOAT.

Need to check autoselect functionality but only available on two sources av_capture_input on macOS and dshow_input on Windows.

I've checked the signature and description of QComboBox::findData are same in between Qt5 and Qt6.
Qt5: https://doc.qt.io/qt-5/qcombobox.html#findData
Qt6: https://doc.qt.io/qt-6/qcombobox.html#findData

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
    • I didn't test autoselect functionality.
    • I didn't test on Windows though especially the behavior of QT_UTF8 is different on Windows.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@RytoEX
Copy link
Member

RytoEX commented Jul 23, 2022

I've checked the signature and description of QComboBox::findData are same in between Qt5 and Qt6.
Qt5: https://doc.qt.io/qt-5/qcombobox.html#findData
Qt6: https://doc.qt.io/qt-6/qcombobox.html#findData

See QTBUG-104990.
cc: @gxalpha

@norihiro
Copy link
Contributor Author

However QTBUG-104990 claims the bug was introduced in Qt6, the issue was found in Qt5. Maybe OBS_COMBO_FORMAT_INT is broken by Qt6 but OBS_COMBO_FORMAT_FLOAT is not working in both Qt5 and Qt6.

@RytoEX RytoEX added the Bug Fix Non-breaking change which fixes an issue label Jul 23, 2022
@RytoEX
Copy link
Member

RytoEX commented Jul 23, 2022

However QTBUG-104990 claims the bug was introduced in Qt6, the issue was found in Qt5. Maybe OBS_COMBO_FORMAT_INT is broken by Qt6 but OBS_COMBO_FORMAT_FLOAT is not working in both Qt5 and Qt6.

I'm saying that the behavior of QComboBox::findData changes between Qt5 and Qt6, which is why I highlighted the text saying that the function is the same between Qt5 and Qt6.

norihiro added a commit to norihiro/obs-color-monitor that referenced this pull request Jul 23, 2022
norihiro added a commit to norihiro/obs-color-monitor that referenced this pull request Jul 25, 2022
norihiro added a commit to norihiro/obs-color-monitor that referenced this pull request Jul 25, 2022
norihiro added a commit to norihiro/obs-color-monitor that referenced this pull request Jul 25, 2022
norihiro added a commit to norihiro/obs-color-monitor that referenced this pull request Jul 25, 2022
UI/properties-view.cpp Outdated Show resolved Hide resolved
UI/properties-view.cpp Outdated Show resolved Hide resolved
@jpark37
Copy link
Collaborator

jpark37 commented Jul 28, 2022

I know I just added two (optional) defects, but please merge this sooner than later. It's quite a nasty bug. The code looks good otherwise.

Copy link
Collaborator

@jpark37 jpark37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the commit message. This is no longer just about OBS_COMBO_FORMAT_FLOAT.

@jpark37 jpark37 added this to the OBS Studio 28.0 milestone Jul 28, 2022
A combobox with `OBS_COMBO_FORMAT_FLOAT` type couldn't be initialized to
select current item from the settings data. This commit will create
QVariant value with the similar code as that of adding the items and
search the current item using that value.
@jpark37
Copy link
Collaborator

jpark37 commented Jul 28, 2022

Looks good to me. Someone merge this ASAP as possible.

@norihiro norihiro changed the title UI: Fix current item on float-type combobox property UI: Search combo property item with QVariant type Jul 28, 2022
Copy link
Member

@gxalpha gxalpha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work fine, code changes make sense to me. In an ideal world, I'd love to hear back from QTBUG-104990 first, but even if it's a bug it would exist for some time so we'd need this anyways, so we should probably just merge it.

@RytoEX RytoEX merged commit 766d1bb into obsproject:master Jul 28, 2022
@norihiro norihiro deleted the ui-combo-float branch July 29, 2022 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants