Some comments about the custominfo SDK:
- For all functions that take a metadb_handle as parameter, you can specify the handle to be null.
This tells custominfo to use the previously accessed storage entry instead of searching for an entry
associated with certain location. This may greatly improve the speed of operations consisting of
many calls. And naturally, lock()/unlock() must be used with this feature.
This is just asking for trouble. You may forget to acquire the lock, or may forget to update the "cursor" correctly in more complex operations. A more robust solution would be to use your own brand of handles that wrap access to the information of a single item much like a metadb_handle wraps access to the cached file metadata in foobar2000 itself.
- Return values for field processing methods are true on success (even if requested location/field not found),
and false on critical failures (after which it will automatically be disabled).
That does not make sense. If something really goes horribly wrong, you can throw an exception. With an exception, you also have the chance to pass information about the failure up to the caller (and lastly to the user).
Raw pointers to service instances (like custominfo or metadb_handle): The smart pointer classes in the SDK weren't just created on a whim, they are meant to reduce programming errors. Using "optimizations" like avoiding their use is unnecessary and only encourages an inconsistent programming style.
- Custominfo core will take care of removing dead/empty entries, updating locations of moved files,
and restricting information storing to media library entries only (according to user settings).
Would it also update inactive backends, or would the information contained in these become outdated over time?
//Returns the name or short description of the storage service in parameter "out".
//It is shown in the drop down list of the configuration dialog, and used for identifying the service.
virtual void get_name(pfc::string_base &out) = 0;
This essentially means that the user will have to reconfigure the component if the name of a storage is ever changed (for whatever reason). If you use a GUID for identification and the name only for the user interface, you can avoid that.
Assuming items in the storage can be accessed efficiently as a linear list: This assumption is wrong for most advanced data structures (including an possible SQL backend). While it is possible to provide iterators for a lot of data structures, the best approach is again to use handles and just ask the storage backend for the handle associated with a given location/metadb_handle.
Updating information in the storage backend on an iten-by-item and even field-by-field basis: That is a performance bottleneck if I ever saw one, especially if you trigger a global update notification for every updated field. Any kind of batch update will be really slow.
custominfo* custominfo::get()
{
static service_ptr_t<custominfo> ptr;
if (ptr==0)
service_enum_create_t(ptr,0);
return ptr.get_ptr();
}
Apart from not being thread-safe, this will crash if the server is unloaded before the client, because at the time the service_ptr_t<> destructor is run, the pointer it contains is no longer valid in that case. It will try to call the release method of that pointer and cause an access violation. You should use static_api_ptr_t<> if you know the service exists (like inside foo_custominfo itself), or use a service_ptr_t<> and manually create the service instance. However, you should manually release that pointer, if the lifetime of the service_ptr_t<> extends beyond initquit::on_quit, which could happen if the service_ptr_t<> is a member of a dialog class.
Topic: foo_custominfo (Read 195949 times)
previous topic - next topic