feat(#6716): ✨ added effect categories - #8493
Conversation
This reverts commit 7145167.
…lter does not select anything except in english
yohannd1
left a comment
There was a problem hiding this comment.
Style review for now. Will do a more in-depth analysis later.
| private: | ||
| static std::unique_ptr<EffectCategory> s_instance; | ||
| QStringList* m_categories; | ||
| QStringList* getCategoriesFromMap(std::map<QString,QString> map); |
There was a problem hiding this comment.
| QStringList* getCategoriesFromMap(std::map<QString,QString> map); | |
| QStringList* getCategoriesFromMap(std::map<QString, QString> map); |
|
|
||
| // Short-hand function | ||
| LMMS_EXPORT EffectCategory* getEffectCategory(); | ||
| } // namespace lmms |
There was a problem hiding this comment.
| } // namespace lmms | |
| } // namespace lmms | |
| EffectCategory* getEffectCategory() | ||
| { return EffectCategory::instance(); } |
There was a problem hiding this comment.
| EffectCategory* getEffectCategory() | |
| { return EffectCategory::instance(); } | |
| EffectCategory* getEffectCategory() { return EffectCategory::instance(); } |
|
|
||
| QStringList* EffectCategory::getCategories() | ||
| { | ||
| if(m_categories == nullptr || m_categories->isEmpty()) |
There was a problem hiding this comment.
| if(m_categories == nullptr || m_categories->isEmpty()) | |
| if (m_categories == nullptr || m_categories->isEmpty()) |
| { | ||
| m_categories = getCategoriesFromMap(lmmsEffects); | ||
| QStringList* ladspaCategories = getCategoriesFromMap(ladspaEffects); | ||
| foreach(QString category, *ladspaCategories) |
There was a problem hiding this comment.
| foreach(QString category, *ladspaCategories) | |
| for (const QString& category : ladspaCategories) |
(Not fully sure about this one, please do try it out)
| QStringList* EffectCategory::getCategoriesFromMap(std::map<QString,QString> map) | ||
| { | ||
| auto* categories = new QStringList(); | ||
| for(auto & it : map) |
There was a problem hiding this comment.
| for(auto & it : map) | |
| for (auto& it : map) |
| auto* categories = new QStringList(); | ||
| for(auto & it : map) | ||
| { | ||
| if(! categories->contains(it.second)) |
There was a problem hiding this comment.
| if(! categories->contains(it.second)) | |
| if (!categories->contains(it.second)) |
| auto labelText = QString{"<p>%1%2</p>" | ||
| "<p>%3%4</p>" | ||
| "<p>%5%6</p>"} | ||
| .arg(tr("<b>Name: </b>"), descriptor.displayName, tr("<b>Author: </b>"), | ||
| QString::fromUtf8(descriptor.author) | ||
| .replace("/dot/", ".") | ||
| .replace("/at/", "@") | ||
| .toHtmlEscaped(), | ||
| tr("<b>Description: </b>"), | ||
| qApp->translate("PluginBrowser", descriptor.description).toHtmlEscaped()); | ||
|
|
There was a problem hiding this comment.
Using this amount of indentation is not recommended, as we use tabs and they account for 8 spaces (pushing too close, or sometimes beyond the 120-character line limit).
I recommend keeping the style the same as what it was before, or at least using a single tab indent ahead of the lines - e.g.:
auto labelText = QString{"<p>%1%2</p>"
"<p>%3%4</p>"
// and so on...| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | ||
| { | ||
| m_model.setEffectTypeFilter(value == tr("All") ? "" : value); | ||
| updateSelection(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | |
| { | |
| m_model.setEffectTypeFilter(value == tr("All") ? "" : value); | |
| updateSelection(); | |
| } | |
| ); | |
| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | |
| { | |
| m_model.setEffectTypeFilter(value == tr("All") ? "" : value); | |
| updateSelection(); | |
| }); |
| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | ||
| { | ||
| m_model.setEffectCategoryFilter(value == tr("All") ? "" : value); | ||
| updateSelection(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | |
| { | |
| m_model.setEffectCategoryFilter(value == tr("All") ? "" : value); | |
| updateSelection(); | |
| } | |
| ); | |
| connect(buttonFilter, &QComboBox::textActivated, this, [this](QString value) | |
| { | |
| m_model.setEffectCategoryFilter(value == tr("All") ? "" : value); | |
| updateSelection(); | |
| }); |
|
Oh, also - I've noticed you're using |
yohannd1
left a comment
There was a problem hiding this comment.
Just a few more style adjustments...
|
|
||
| // Short-hand function | ||
| LMMS_EXPORT EffectCategory* getEffectCategory(); | ||
| } |
There was a problem hiding this comment.
| } | |
| } // namespace lmms | |
| layout->addWidget(label); | ||
| layout->addWidget(buttonFilter); | ||
| return layout; | ||
| } |
|
Okay, now a proper review.
I believe yes, at least for these, which are predefined. I don't think it's needed for custom categories.
Pretty sure there is, though I don't know the specific details. I do know that translatable strings are implemented through the Also, a question: are you planning to implement custom (and multiple) categories on this PR? I think it should be implemented together with this, and I believe it won't require too many changes. For custom categories, what I propose is a "category manager window" which lists all categories and allows you to add custom categories. And I think saving them on a XML (as you mentioned on Discord) will probably be the best bet here. I'd say saving them on the As for multiple categories on a plugin (a.k.a. tags), I feel like it's gonna change the filtering logic significantly, from the way the filtering seems to work on Qt tables. |
| return m_categories; | ||
| } | ||
|
|
||
| QStringList* EffectCategory::getCategoriesFromMap(std::map<QString, QString> map) |
There was a problem hiding this comment.
I believe this can be returned as a plain QStringList. Qt does implicit sharing with this class, plus using the pointer entails manually freeing it later (since it's not attached to a QObject), and that doesn't seem to be done at the moment.
There was a problem hiding this comment.
Oh, that's neat, I did not know that ! I made some changes for this. If I can let the computer deal with the memory management, that's for the better.
…t category getters to non pointers
Okay, so I added For the custom category, I think it would be best if I do that in another PR. I tend to hop between projects, and I might do the other steps later (in a few weeks probably), but I wanted this dev to still be an improvement that could be merged (even if it's not great yet). That might also give users some time to give some feedbacks. I will probably add the custom category next, but I rather thought to do that with a menu appearing on a right click. I might also add another column dedicated to favorites in the meantime (kind of boolean style column, with a star appearing if it is a favorite with the associated filter) since I'm starting to understand how it works. After that I will probably try to add the multiple categories to an effect, but I'm having a hard time to make it work UI-wise (should every category appear on the same line ? Is there a limit ? Should it be on multiple lines ? Etc.). |
Did you mean for using
Sadly PRs in LMMS take quite a long time to be merged, due to the small amount of maintainers and free time. This PR won't probably be merged in a while yet.
Oh, this sounds good to me!
I think they should appear on the same line. It might make things harder to read, yeah. Maybe just adding |
Yes, that's exactly that, and yes, that makes complete sense now that you mention it.
Yes, okay, that's fair. I will still work in another dedicated branch once I get back to it and maybe merge it to this one if it is not merged yet then. |
# Changes
# Notes for the reviewer(s)
EffectKeyListtype to display the effect, which is not directly compatible with theEffecttype and is also used for other plugin types (instruments, etc.)# Screenshot of the feature