-
Notifications
You must be signed in to change notification settings - Fork 47
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
Report inserting media, including items with adjusted playrate #1104
base: master
Are you sure you want to change the base?
Conversation
Shout out to @Timtam for his help with how to store the item information, and his unending patience every time I forget how to write loops. |
Build succeeded! Build osara pr1104-1476,b88fc86d completed (commit b88fc86d95 by @ScottChesworth) |
Do sighted folks get any immediate indication that this rate adjustment has happened? I'm all for making things more efficient, but I'm not quite following how screen reader users are at a disadvantage here compared to others. |
int oldItems = CountMediaItems(0); | ||
set<MediaItem*> oldItems; | ||
for (int i = 0; i < CountMediaItems(0); i++) { | ||
oldItems.insert(GetMediaItem(0, i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that this (and the loop below) could cause performance problems in really large projects. This isn't just walking selected media items or items on the current track; it's walking through every item in the entire project. While I understand why that is the case, walking through potentially thousands of items twice every time one pastes seems like it might become problematic. It's not necessarily a show stopper, but I'd like some assurance that this has been verified to be snappy in a massive project before we consider merging.
rateAdjusted++; | ||
} | ||
if (rateAdjusted > 0) | ||
s << ", " << format(translate_plural("{} item rate adjusted", "{} items rate adjusted", rateAdjusted), rateAdjusted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translators comment please.
@jcsteh yeah, I thought of that while implementing it but didn't came up with a better solution. I checked if the zero-based index solution would work, but REAPER sorts items based on tracks from top to bottom instead of appending new items to the end, so that unfortunately was out for this solution. I hope that we're talking about a O(1) implementation when mapping the zero-based index to an item id, in which case the loop would still be fast enough to not cause any performance issues at all, but obviously I don't know. We'd optimally need a project with at least a thousand items (preferably more) to see how they perform under heavy load circumstances. |
I just tried it in a project with 5000 items spread over 50 tracks. No noticeable lag pasting or inserting here. That said, we might not need this anyway because it looks like Cockos are gonna do more tweaking of the import options. |
I'd say you're right about an O(1) index lookup, but that's still an O(n) loop. With a sufficiently large n, that could still be a problem. Scott's test suggest that n = 5000 seems to be okay, so that should be fine, though we'd need to watch closely for reports of problems. I'd still like to know whether sighted users can immediately spot this. Otherwise, this would appear to be a REAPER problem, not something we should try to solve in OSARA. Based on Scott's comment, it seems like Cockos might agree. |
With two loops that would be O(2n), which should be just fine with just a few thousand items, but you're right, lets keep this open and wait for Cockos to make their move. I don't think that you actually get a visual indicator when importing items that way, I however don't know if items generally get a visual indicator of sorts if the play rate is anything other than 1.0, but I doubt it. |
This makes OSARA report a summary of tracks/items added when using REAPER's "Insert media files" action, it also checks whether any of those newly inserted items have had their playrate adjusted and reports it. I added the playrate adjustment because preferences around it are changing, the defaults for new prefs are a bit heavy-handed IMO, have already seen it catching a couple of users unawares.