[vlc-commits] [Git][videolan/vlc][master] 8 commits: macos: cache subcat-is-general check result
Hugo Beauzée-Luyssen (@chouquette)
gitlab at videolan.org
Fri Dec 24 10:52:11 UTC 2021
Hugo Beauzée-Luyssen pushed to branch master at VideoLAN / VLC
Commits:
c62b1b5b by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: cache subcat-is-general check result
- - - - -
06297c6e by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: fix potential prefs tree option misplacement
when processing the core option set, if the subcat is a 'general' one and
we have a category node to attach the current option to, we attached it.
the alternate path was intended for otherwise attaching options of
non-general subcats to the relevant subcat node (when we have a node for
that subcat).
the problem with the implementation of this is what happens if the subcat
is considered a 'general' one, but should we not have a cat node. in this
case it would go down the incorrect path of trying to attach the option to
the last created subcat node, rather than performing the correct action of
ignoring it.
- - - - -
232de8f7 by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: merge prefs tree node types
nodes for plugins are created in the preferences tree depending upon use of
'subcategory' entries within plugin option sets. while most plugins specify
'non-general' subcategories, and thus will appear on the 3rd tier of the
tree (cat -> subcat -> plugin), some specify 'general' subcategories (which
represent option sets shown when category nodes are selected), and in this
case the plugin node must appear on the 2nd tier alongside subcategory
nodes.
the existing code here did not support this however, with such nodes
instead mistakenly being attached to the previous non-general subcat node,
if available, which is completely wrong.
unfortunately implementation of a simple fix for this is blocked because
different object types are used to represent each of the three different
node types (category; subcategory; plugin) and everything is naively
designed upon the false expectation that the three tiers of the tree can
map purely to each of these respective object types, incompatible with
allowing for a mix of subcat and plugin nodes on the 2nd tier.
a significant overhaul of the node design is thus first required before we
can fix the actual misplacement issue. here i have taken the obvious
approach of unifying the three branch types into one, with an enum based
property identifying subtypes. yes, this requires a little extra memory,
but it's simple and gives us the needed flexibility.
the misplacement issue itself will be fixed in the next commit.
note, a few additional lines where changed in taking the opportunity to
rename `subCategoryItem` to `subcategoryItem`.
- - - - -
84e55fc9 by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: fix placement of plugin node's targeting general subcats
as explained in the previous commit, plugins that reference 'general'
subcategories in their option sets are mishandled when it comes to creating
nodes in the plugin tree. the previous commit laid the groundwork for a
solution to this, and now here we actually fix the problem.
to summarise, if the target subcat is a general one, the plugin node is
supposed to be attached to the cat node, i.e. placed on the 2nd tier
alongside subcategory nodes for that category. instead it was mistakenly
attached to the previous `subCategoryItem`.
in fact if the first or only subcat specified in the plugin's option set is
a general one, `subCategoryItem` would be null, and thus the plugin node
object would just never be attached at all.
- - - - -
786fb23f by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: fix multi-free unsoundness and duplicate plugin nodes in prefs tree
whilst most plugin descriptors make only a single `set_subcategory()` call
(indicating relevant location in the preferences tree for their options),
some make multiple calls, generating multiple 'subcat' entries within their
option sets.
it should be noted that the qt prefs tree construction code (currently)
handles this by essentially ignoring all but the first one; whether or not
this is the best design, vs. breaking up such option sets across the tree,
is a subject that is tackled in a piece of work to be submitted for review
later.
in any case, the macos prefs tree construction code here looked highly
suspect to me with regards to how it handles such situations. (i'm basing
this on reading code only, i have no Apple device available).
the loop processing each plugin:
1. creates a `PluginBranch` item. this holds the copy of the config set
returned by the `module_config_get()` call, which its destructor will
`free()`.
2. as the plugin's config set is iterated over, cat and subcat nodes are
created as necessary (if nodes for them have not already been created)
in direct response to any subcat entries (to ensure the existence of
the parent nodes that the plugin node will be attached to in the tree).
3. for other item types in the set (i.e. actual options along with
'section' items), they are 'attached' to the `PluginBranch` item (added
to it's `options` array). additionally the `PluginBranch` item itself
is 'attached' to the relevant cat/subcat node (added to that node's
`children` array) if it is not already present there.
so when it comes to plugin option sets that contain multiple subcat items,
this logic means that the `PluginBranch` object will get 'attached' once to
each and every unique subcat in the set (well, those that are directly
followed by at least one non-subcat item).
this fact alone is a problem, since we surely don't want copies of a
plugin's entire option set appearing in multiple places in the tree. it
could be a source of confusion, and creates a potential problem (which i've
not properly examined) when it comes to saving changes.
however it is worse than just an organisational problem - this duplication
of `PluginBranch` instances within the tree actually creates multi-free
issues when it comes to the matter of object destruction. when 'attaching'
`PluginBranch` objects to a cat/subcat we are adding its pointer to an
`NSMutableArray`, which apparently automatically calls `release` (the obj-C
equivalent of C++'s `delete`) on the items it contains when it itself is
destroyed. effectively, by placing the pointer of an allocated object into
such an array, we hand over ownership. the problem is that we are assigning
ownership of the same object to multiple arrays in these multi-subcat
cases. this means that these objects get released multiple times, and
furthermore via their destructors free their config sets multiple times.
ouch!
this commit fixes the problem very simply by keeping track of whether or
not we have already attached the plugin node somewhere, and only attaches
it to a cat/subcat if not. this prevents the issues described here, and
brings the handling of such multi-subcat option sets more into line with
the current qt handling.
an issue remains of potentially unnecessary cat/subcat node creation, which
will be tackled in a later commit.
- - - - -
54f91e7f by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: add method for prefs tree leaf object creation
as done with cat/subcat/plugin objects.
- - - - -
1a0652ca by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: catch failed option/plugin object creation in prefs tree construction
- - - - -
c9965e65 by Lyndon Brown at 2021-12-24T10:38:11+00:00
macos: rework prefs tree construction logic
- fixes the possibility of cat and subcat nodes sometimes being created
when not needed.
- more efficiently skips things when cat is hidden or unknown.
- - - - -
1 changed file:
- modules/gui/macosx/preferences/prefs.m
Changes:
=====================================
modules/gui/macosx/preferences/prefs.m
=====================================
@@ -102,39 +102,36 @@
@end
-/* CONFIG_SUBCAT */
- at interface VLCTreeSubCategoryItem : VLCTreeItem
-{
- int _subCategory;
-}
-+ (VLCTreeSubCategoryItem *)subCategoryTreeItemWithSubCategory:(int)subCategory;
-- (id)initWithSubCategory:(int)subCategory;
-- (int)subCategory;
- at end
+enum VLCTreeBranchType {
+ CategoryBranch = 0,
+ SubcategoryBranch = 1,
+ PluginBranch = 2,
+};
-/* Plugin daughters */
- at interface VLCTreePluginItem : VLCTreeItem
+ at interface VLCTreeBranchItem : VLCTreeItem
{
+ enum VLCTreeBranchType _branchType;
+ int _category;
+ int _subcategory;
+ /* for plugin type */
module_config_t * _configItems;
unsigned int _configSize;
}
-+ (VLCTreePluginItem *)pluginTreeItemWithPlugin:(module_t *)plugin;
-- (id)initWithPlugin:(module_t *)plugin;
++ (VLCTreeBranchItem *)newCategoryTreeBranch:(int)category;
++ (VLCTreeBranchItem *)newSubcategoryTreeBranch:(int)subcategory;
++ (VLCTreeBranchItem *)newPluginTreeBranch:(module_t *)plugin;
-- (module_config_t *)configItems;
-- (unsigned int)configSize;
- at end
-
-/* CONFIG_CAT */
- at interface VLCTreeCategoryItem : VLCTreeItem
-{
- int _category;
-}
-+ (VLCTreeCategoryItem *)categoryTreeItemWithCategory:(int)category;
- (id)initWithCategory:(int)category;
+- (id)initWithSubcategory:(int)subcategory;
+- (id)initWithPlugin:(module_t *)plugin;
+
+- (VLCTreeBranchItem *)childRepresentingSubcategory:(int)category;
+- (enum VLCTreeBranchType)branchType;
- (int)category;
-- (VLCTreeSubCategoryItem *)itemRepresentingSubCategory:(int)category;
+- (int)subcategory;
+- (module_config_t *)configItems;
+- (unsigned int)configSize;
@end
/* individual options. */
@@ -142,12 +139,16 @@
{
module_config_t * _configItem;
}
++ (VLCTreeLeafItem *)newTreeLeaf:(module_config_t *)configItem;
- (id)initWithConfigItem:(module_config_t *)configItem;
- (module_config_t *)configItem;
@end
- at interface VLCTreeMainItem : VLCTreePluginItem
-- (VLCTreeCategoryItem *)itemRepresentingCategory:(int)category;
+ at interface VLCTreeMainItem : VLCTreeItem
+{
+ module_config_t * _configItems;
+}
+- (VLCTreeBranchItem *)childRepresentingCategory:(int)category;
@end
#pragma mark -
@@ -445,13 +446,13 @@
#pragma mark -
@implementation VLCTreeMainItem
-- (VLCTreeCategoryItem *)itemRepresentingCategory:(int)category
+- (VLCTreeBranchItem *)childRepresentingCategory:(int)category
{
NSUInteger childrenCount = [[self children] count];
for (int i = 0; i < childrenCount; i++) {
- VLCTreeCategoryItem * categoryItem = [[self children] objectAtIndex:i];
- if ([categoryItem category] == category)
- return categoryItem;
+ VLCTreeBranchItem * item = [[self children] objectAtIndex:i];
+ if ([item branchType] == CategoryBranch && [item category] == category)
+ return item;
}
return nil;
}
@@ -471,12 +472,15 @@
/* Build a tree of the plugins */
/* Add the capabilities */
for (i = 0; i < count; i++) {
- VLCTreeCategoryItem * categoryItem = nil;
- VLCTreeSubCategoryItem * subCategoryItem = nil;
- VLCTreePluginItem * pluginItem = nil;
+ VLCTreeBranchItem * categoryItem = nil;
+ VLCTreeBranchItem * subcategoryItem = nil;
+ VLCTreeBranchItem * pluginItem = nil;
module_config_t *p_configs = NULL;
- int lastcat = CAT_UNKNOWN;
- int lastsubcat = SUBCAT_UNKNOWN;
+ int cat = CAT_UNKNOWN;
+ int subcat = SUBCAT_UNKNOWN;
+ bool subcat_is_general = false;
+ bool plugin_node_added = false;
+ bool pending_tree_node_creation = false;
unsigned int confsize;
module_t * p_module = modules[i];
@@ -485,74 +489,90 @@
/* Exclude empty plugins (submodules don't have config */
/* options, they are stored in the parent module) */
if (mod_is_main) {
- pluginItem = self;
_configItems = module_config_get(p_module, &confsize);
- _configSize = confsize;
+ p_configs = _configItems;
} else {
- pluginItem = [VLCTreePluginItem pluginTreeItemWithPlugin: p_module];
+ pluginItem = [VLCTreeBranchItem newPluginTreeBranch: p_module];
+ if (!pluginItem)
+ continue;
confsize = [pluginItem configSize];
+ p_configs = [pluginItem configItems];
}
- p_configs = [pluginItem configItems];
for (unsigned int j = 0; j < confsize; j++) {
int configType = p_configs[j].i_type;
if (configType == CONFIG_SUBCATEGORY) {
- lastsubcat = (int)p_configs[j].value.i;
- if( lastsubcat == SUBCAT_HIDDEN ) {
- categoryItem = nil;
- subCategoryItem = nil;
- continue;
- }
- lastcat = vlc_config_cat_FromSubcat(lastsubcat);
+ subcat = (int) p_configs[j].value.i;
+ cat = vlc_config_cat_FromSubcat(subcat);
+ subcat_is_general = vlc_config_subcat_IsGeneral(subcat);
+ pending_tree_node_creation = true;
+ continue;
+ }
+
+ if (cat == CAT_HIDDEN || cat == CAT_UNKNOWN)
+ continue;
+
+ if (!CONFIG_ITEM(configType) && configType != CONFIG_SECTION)
+ continue;
- categoryItem = [self itemRepresentingCategory:lastcat];
+ VLCTreeLeafItem *new_leaf = [VLCTreeLeafItem newTreeLeaf:&p_configs[j]];
+ if (!new_leaf) continue;
+
+ if (!plugin_node_added && pending_tree_node_creation) {
+ categoryItem = [self childRepresentingCategory:cat];
if (!categoryItem) {
- categoryItem = [VLCTreeCategoryItem categoryTreeItemWithCategory:lastcat];
+ categoryItem = [VLCTreeBranchItem newCategoryTreeBranch:cat];
if (categoryItem)
[[self children] addObject:categoryItem];
+ else
+ continue;
}
- if (categoryItem && !vlc_config_subcat_IsGeneral(lastsubcat)) {
- subCategoryItem = [categoryItem itemRepresentingSubCategory:lastsubcat];
- if (!subCategoryItem) {
- subCategoryItem = [VLCTreeSubCategoryItem subCategoryTreeItemWithSubCategory:lastsubcat];
- if (subCategoryItem)
- [[categoryItem children] addObject:subCategoryItem];
+ if (!subcat_is_general) {
+ subcategoryItem = [categoryItem childRepresentingSubcategory:subcat];
+ if (!subcategoryItem) {
+ subcategoryItem = [VLCTreeBranchItem newSubcategoryTreeBranch:subcat];
+ if (subcategoryItem)
+ [[categoryItem children] addObject:subcategoryItem];
+ else
+ continue;
}
}
- continue;
- }
-
- if (!CONFIG_ITEM(configType) && configType != CONFIG_SECTION)
- continue;
- if (mod_is_main) {
- if (categoryItem && vlc_config_subcat_IsGeneral(lastsubcat)) {
- [[categoryItem options] addObject:[[VLCTreeLeafItem alloc] initWithConfigItem:&p_configs[j]]];
- } else if (subCategoryItem) {
- [[subCategoryItem options] addObject:[[VLCTreeLeafItem alloc] initWithConfigItem:&p_configs[j]]];
- }
- }
- else {
- if (subCategoryItem && ![[subCategoryItem children] containsObject: pluginItem]) {
- [[subCategoryItem children] addObject:pluginItem];
+ if (!mod_is_main) {
+ if (subcat_is_general)
+ [[categoryItem children] addObject:pluginItem];
+ else
+ [[subcategoryItem children] addObject:pluginItem];
+ plugin_node_added = true;
}
- if (pluginItem) {
- [[pluginItem options] addObject:[[VLCTreeLeafItem alloc] initWithConfigItem:&p_configs[j]]];
- }
+ pending_tree_node_creation = false;
}
+
+ if (mod_is_main && subcat_is_general)
+ [[categoryItem options] addObject:new_leaf];
+ else if (mod_is_main)
+ [[subcategoryItem options] addObject:new_leaf];
+ else
+ [[pluginItem options] addObject:new_leaf];
}
}
module_list_free(modules);
return _children;
}
+
+- (void)dealloc
+{
+ if (_configItems)
+ module_config_free(_configItems);
+}
@end
#pragma mark -
- at implementation VLCTreeCategoryItem
-+ (VLCTreeCategoryItem *)categoryTreeItemWithCategory:(int)category
+ at implementation VLCTreeBranchItem
++ (VLCTreeBranchItem *)newCategoryTreeBranch:(int)category
{
if (!vlc_config_cat_GetName(category)) {
msg_Err(getIntf(), "failed to get name for category %i", category);
@@ -561,82 +581,91 @@
return [[[self class] alloc] initWithCategory:category];
}
++ (VLCTreeBranchItem *)newSubcategoryTreeBranch:(int)subcategory
+{
+ if (!vlc_config_subcat_GetName(subcategory))
+ return nil;
+ return [[[self class] alloc] initWithSubcategory:subcategory];
+}
+
++ (VLCTreeBranchItem *)newPluginTreeBranch:(module_t *)plugin
+{
+ return [[[self class] alloc] initWithPlugin:plugin];
+}
+
- (id)initWithCategory:(int)category
{
NSString * name = _NS(vlc_config_cat_GetName(category));
if (self = [super initWithName:name]) {
+ _branchType = CategoryBranch;
_category = category;
+ _subcategory = SUBCAT_UNKNOWN;
+ _configItems = nil;
+ _configSize = 0;
//_help = [_NS(vlc_config_cat_GetHelp(category)) retain];
}
return self;
}
-- (VLCTreeSubCategoryItem *)itemRepresentingSubCategory:(int)subCategory
+- (id)initWithSubcategory:(int)subcategory
{
- assert([self isKindOfClass:[VLCTreeCategoryItem class]]);
- NSUInteger childrenCount = [[self children] count];
- for (NSUInteger i = 0; i < childrenCount; i++) {
- VLCTreeSubCategoryItem * subCategoryItem = [[self children] objectAtIndex:i];
- if ([subCategoryItem subCategory] == subCategory)
- return subCategoryItem;
+ NSString * name = _NS(vlc_config_subcat_GetName(subcategory));
+ if (self = [super initWithName:name]) {
+ _branchType = SubcategoryBranch;
+ _category = CAT_UNKNOWN;
+ _subcategory = subcategory;
+ _configItems = nil;
+ _configSize = 0;
+ //_help = [_NS(vlc_config_subcat_GetHelp(subcategory)) retain];
}
- return nil;
-}
-
-- (int)category
-{
- return _category;
+ return self;
}
- at end
-
-#pragma mark -
- at implementation VLCTreeSubCategoryItem
-- (id)initWithSubCategory:(int)subCategory
+- (id)initWithPlugin:(module_t *)plugin
{
- NSString * name = _NS(vlc_config_subcat_GetName(subCategory));
+ NSString * name = _NS(module_get_name(plugin, false));
if (self = [super initWithName:name]) {
- _subCategory = subCategory;
- //_help = [_NS(vlc_config_subcat_GetHelp(subCategory)) retain];
+ _branchType = PluginBranch;
+ _category = CAT_UNKNOWN;
+ _subcategory = SUBCAT_UNKNOWN;
+ _configItems = module_config_get(plugin, &_configSize);
+ //_plugin = plugin;
+ //_help = [_NS(vlc_config_subcat_GetHelp(subcategory)) retain];
}
return self;
}
-+ (VLCTreeSubCategoryItem *)subCategoryTreeItemWithSubCategory:(int)subCategory
+- (void)dealloc
{
- if (!vlc_config_subcat_GetName(subCategory))
- return nil;
- return [[[self class] alloc] initWithSubCategory:subCategory];
+ if (_configItems)
+ module_config_free(_configItems);
}
-- (int)subCategory
+- (VLCTreeBranchItem *)childRepresentingSubcategory:(int)subcategory
{
- return _subCategory;
+ assert([self isKindOfClass:[VLCTreeBranchItem class]]);
+ NSUInteger childrenCount = [[self children] count];
+ for (NSUInteger i = 0; i < childrenCount; i++) {
+ VLCTreeBranchItem * item = [[self children] objectAtIndex:i];
+ if ([item branchType] == SubcategoryBranch && [item subcategory] == subcategory)
+ return item;
+ }
+ return nil;
}
- at end
-
-#pragma mark -
- at implementation VLCTreePluginItem
-- (id)initWithPlugin:(module_t *)plugin
+- (enum VLCTreeBranchType)branchType
{
- NSString * name = _NS(module_GetShortName(plugin));
- if (self = [super initWithName:name]) {
- _configItems = module_config_get(plugin, &_configSize);
- //_plugin = plugin;
- //_help = [_NS(vlc_config_subcat_GetHelp(subCategory)) retain];
- }
- return self;
+ return _branchType;
}
-+ (VLCTreePluginItem *)pluginTreeItemWithPlugin:(module_t *)plugin
+- (int)category
{
- return [[[self class] alloc] initWithPlugin:plugin];
+ return _category;
}
-- (void)dealloc
+- (int)subcategory
{
- module_config_free(_configItems);
+ return _subcategory;
}
- (module_config_t *)configItems
@@ -653,6 +682,10 @@
#pragma mark -
@implementation VLCTreeLeafItem
++ (VLCTreeLeafItem *)newTreeLeaf:(module_config_t *)configItem
+{
+ return [[[self class] alloc] initWithConfigItem:configItem];
+}
- (id)initWithConfigItem: (module_config_t *) configItem
{
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/d0547192a7f7433f9afc64a81dd7264536558f9e...c9965e651b85005c2a526fd62f5b236cd620430d
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/d0547192a7f7433f9afc64a81dd7264536558f9e...c9965e651b85005c2a526fd62f5b236cd620430d
You're receiving this email because of your account on code.videolan.org.
More information about the vlc-commits
mailing list