-
-
Notifications
You must be signed in to change notification settings - Fork 994
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
Make ControlLayout
more general
#6456
base: master
Are you sure you want to change the base?
Conversation
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/72d2578a-4c91-46f0-a396-60df9dd0b937/artifacts/0/lmms-1.3.0-alpha.1.211+gbe3f603c1-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17683?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/a31cfcf2-86bc-486c-94ab-cd05beefd665/artifacts/0/lmms-1.3.0-alpha.1.211+gbe3f603c1-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17684?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/637e56e8-94fa-4bca-a3a5-24df55dfca1a/artifacts/0/lmms-1.3.0-alpha.1.211+gbe3f603c1-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17681?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/evx3g90w9fegot3b/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44060299"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/b238ocnbq7uamrfw/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44060299"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/7224cc07-13cc-455a-905d-da4daca29097/artifacts/0/lmms-1.3.0-alpha.1.211+gbe3f603c1-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17682?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "4fe63705f3f966c28f812a0453413be1d9374498"} |
Looking at the plan, without looking too close at the code:
The new search field just looks overlarge to me. Usually, typing 2 or 3 letters will bring you the desired widget, and I'm sure no one ever typed more than 10 letters. The current one has space for 100 letters. I guess if we add a combo box for sorting options or something like that, at least it will get smaller. But for design reasons, I'd even now prefer the widget smaller. Not 100% sure why you moved the widget out of the grid layout. Is it in order to be able to have one search bar for e.g. two layouts (left<->right)?
That makes very much sense.
I don't get what you mean by "affects other text fields in the program". However, agreeing that they are not used. I used them while developing the core, but then stopped using them, forgetting to remove them.
Unfortunately, they are used in #4662 , which I still actively use (until Lv2 Gui will be ported). Also, this seems to work well, I tested it (not sure what you mean by "looked up by id". No objections with the rest. |
Yeah, I was intending to shrink the search box once other sorting and filtering controls are added to the right of it. But I can make it smaller for now if you'd prefer.
For two reasons. One, as you said, to allow one search bar to filter multiple layouts. These could be for left/right processors, or for different categories of control (for VST3 in particular, which allows plugins to expose their internal control arrangements to the host). The other is that I think it's better UX to separate the search bar from the widgets being searched. I can't think of any other search feature, in LMMS or elsewhere, that combines them. (As an added bonus, it makes the code nicer as the search bar no longer needs to be handled as a special case.)
I may be misremembering completely, but I think the motivation for removing the focus from the search bar was to allow playing notes with the computer keyboard again. But this problem exists for all text boxes in the program - if you want to play notes with the keyboard after typing in a text box, you need to focus another subwindow in order to remove keyboard focus from the text box. While I agree that the UX can be improved here, I think a generic method to remove focus from any text box would be the way to go.
When a widget is added to a void ControlLayout::addItem(QLayoutItem *item)
{
QWidget* widget = item->widget();
const QString str = widget ? widget->objectName() : QString("unnamed");
m_itemMap.insert(str, item);
invalidate();
} When a void LinkedModelGroupView::addControl(Control* ctrl, const std::string& id,
const std::string &display, bool removable)
{
// ...
// required, so the Layout knows how to sort/filter widgets by string
box->setObjectName(QString::fromStdString(display));
m_layout->addWidget(box);
// take ownership of control and add it
m_widgets.emplace(id, std::unique_ptr<Control>(ctrl));
// ...
} But when removing a control from a void LinkedModelGroupView::removeControl(const QString& key)
{
auto itr = m_widgets.find(key.toStdString());
if (itr != m_widgets.end())
{
QLayoutItem* item = m_layout->itemByString(key);
// ...
} This can only work if the same value is passed to If you're still using this, I can fix it to work with my changes instead of removing it. |
Ah, I see. I indeed use it with always the same value for both Asides from that, I think I understood your basic plan. When you've pushed the fixes for |
Additional recommendation from #6798: Can we make the search field and the "Help" button share the same line? |
I just see that I have a review pending here. I will try to look at this PR in the next days (though it generally looks good). |
Would it make sense for the text box and the "help" button to be on the same line, seperated by a space. I believe it will improve space utilisation a bit. |
There are a couple of things I want to do with this class: I want to be able to filter multiple layouts with a single search field (for VST3), and I want to be able to sort the control in various ways (e.g. most recently changed, for VST2). These are the changes in this PR:
QLineEdit
, so we can add the sorting options to it in the future.unique_ptr
s rather than raw pointers. The layout owns its layout items, and as they do not derive fromQObject
and so Qt's ownership model does not apply, this is the better choice.removeFocusFromSearchBar
functions. They weren't used, and the problem they were intended to solve affects other text fields in the program, so a more general solution would be better.ControlLayout::itemByString
andLinkedModelGroupView::removeControl
. This was broken, as controls were stored by filter key, but looked up by id. A correct implementation should go somewhere else, as it should not be the layout's responsibility to maintain a separate id-widget map for removing controls. It was also unused.LinkedModelGroupView::addControl
to take the control in aunique_ptr
, since it assumes unique ownership of the control. (Not related to the rest of this PR, and I can revert if you want, but I had already made the change before realising it was unnecessary.)ControlLayout
(andControlFilterWidget
) classes, so they can be used by plugins.Screenshots of an LV2 control dialog before and after this change:
![Screenshot 2022-06-24 190243](https://user-images.githubusercontent.com/1873225/177040170-3ecc9467-c099-4a71-b28f-6980d9adc99c.png)
![Screenshot 2022-07-03 123403](https://user-images.githubusercontent.com/1873225/177040174-0987e039-c823-493c-829a-a56efb2696de.png)
(Yes, the filter box in the new dialog doesn't reach all the way across, but the widget that contains it has the same problem in the former dialog; it's just not visible as no control is wide enough to demonstrate the problem.)