UI: Can copy-paste filters to multiple sources by bershanskiy · Pull Request #4334 · obsproject/obs-studio

bershanskiy · 2021-03-11T21:51:40Z

Description

Fix #4290.
Add support for pasting copied filters to multiple selected sources, instead of only one which was selected first. Instead of calling GetCurrentSceneItem() which returns the index of the first selected source from the array returned by ui->sources->selectionModel()->selectedIndexes(), call ui->sources->selectionModel()->selectedIndexes() directly and iterate over all selected sources.

Adds confirmation prompt if filter is pasted to more than 4 sources:

Motivation and Context

Reporter of the original bug #4290, @AskMP, works with large numbers of sources. In one comment he mentioned having to copy-paste filters to 48 sources by hand.

How Has This Been Tested?

I a bunch of tests, here are a few:

Created many sources (images)
Added a filter to one source which is very obvious
Copied filter from source A, selected one other source B and pasted the filter to it (supported prior to the change)
Copied filter from source A and pasted it to the same source A, checked that filter was not duplicated (supported prior to the change)
Copied filter from source A, selected multiple other sources B,C,D and pasted the filter to them, checked that each got the filter
Copied filter from source A, selected multiple other sources, deselected some of them, pasted the filter, checked that deselected sources did not get the filter

Test env:
OS: Windows 10 Home 20H2
CPU: Intel(R) Core(TM) i7-10875H
GPU: Nvidia GeForce RTX 2060

It does not affect any other functions because I changed only OBSBasic::on_actionPasteFilters_triggered() (I considered making a helper function for getting all selected scene items analogous to GetCurrentSceneItem(), but did not find any other function that could benefit from this helper.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code has been run through clang-format.
I have read the contributing document.
My code is not on the master branch.
The code has been tested.
All commit messages are properly formatted and commits squashed where appropriate.
I have included updates to all appropriate documentation.

WizardCM · 2021-03-11T22:43:42Z

The code looks good, but I wonder if it'd be worth warning the user about what they're about to do, especially if they select more than 4 or 5 sources - especially as there currently isn't an Undo button.

Additionally, would it be worth making sure the source for the sceneitem is unique, so that the same filters aren't copied to the same source multiple times?

RytoEX · 2021-03-12T21:47:03Z

The code looks good, but I wonder if it'd be worth warning the user about what they're about to do, especially if they select more than 4 or 5 sources - especially as there currently isn't an Undo button.

Additionally, would it be worth making sure the source for the sceneitem is unique, so that the same filters aren't copied to the same source multiple times?

Is that not what this section does?
https://github.com/obsproject/obs-studio/pull/4334/files#diff-4c0a52244f264feca9c3031c90edcbbbd1087a3040c15f9df4ca9700c5154fa3R7883-R7884

bershanskiy · 2021-03-12T21:52:21Z

The code looks good, but I wonder if it'd be worth warning the user about what they're about to do, especially if they select more than 4 or 5 sources - especially as there currently isn't an Undo button.
Additionally, would it be worth making sure the source for the sceneitem is unique, so that the same filters aren't copied to the same source multiple times?

Is that not what this section does?
https://github.com/obsproject/obs-studio/pull/4334/files#diff-4c0a52244f264feca9c3031c90edcbbbd1087a3040c15f9df4ca9700c5154fa3R7883-R7884

Not exactly. The check ensures that the filter is not pasted to the source it was copied from, but we also should ensure that if two "destination" scene items have the same source, we apply the filter only once. I'm writing the updated patch right now.

RytoEX · 2021-03-12T22:02:54Z

An OBS Source can be used to create more than one scene item ("Add Existing" or "Paste Reference"), so getting an OBS source from multiple scene items that use the same source should result in the same OBSSource. I've just tested the current code and I didn't end up with duplicate filters pasted onto another scene item that uses the same OBS Source. Unless I'm missing something?

RytoEX · 2021-03-12T22:25:04Z

It looks like remove_items in UI/window-basic-main.cpp has an implementation of getting selected items. It is only used as a callback for obs_scene_enum_items just below it in OBSBasic::on_actionRemoveSource_triggered() for removing multiple selected sources.

Should we rename it to something more appropriate and reuse that?

obs-studio/UI/window-basic-main.cpp

Lines 5234 to 5245 in fc6e085

    
           static bool remove_items(obs_scene_t *, obs_sceneitem_t *item, void *param) 
        
           { 
        
           	vector<OBSSceneItem> &items = 
        
           		*reinterpret_cast<vector<OBSSceneItem> *>(param); 
        
           	if (obs_sceneitem_selected(item)) { 
        
           		items.emplace_back(item); 
        
           	} else if (obs_sceneitem_is_group(item)) { 
        
           		obs_sceneitem_group_enum_items(item, remove_items, &items); 
        
           	} 
        
           	return true; 
        
           };

obs-studio/UI/window-basic-main.cpp

Lines 5247 to 5251 in fc6e085

    
           void OBSBasic::on_actionRemoveSource_triggered() 
        
           { 
        
           	vector<OBSSceneItem> items; 
        
           	obs_scene_enum_items(GetCurrentScene(), remove_items, &items);

bershanskiy · 2021-03-12T22:29:15Z

I've just tested the current code and I didn't end up with duplicate filters pasted onto another scene item that uses the same OBS Source. Unless I'm missing something?

Yes, my initial approach duplicated the filter in the following situation:

Create new empty scene
Create two different sources A and B1
Create a "reference" source B2 referring to B1
Add a filter to A
Copy filter from A
Paste copied filter to B1 and B2 in one step
Observe that B1 and B2 have two copies of the filter

RytoEX · 2021-03-12T22:46:30Z

I've just tested the current code and I didn't end up with duplicate filters pasted onto another scene item that uses the same OBS Source. Unless I'm missing something?

Yes, my initial approach duplicated the filter in the following situation:
1. Create new empty scene
2. Create two different sources A and B1
3. Create a "reference" source B2 referring to B1
4. Add a filter to A
5. Copy filter from A
6. Paste copied filter to B1 and B2 in one step
7. Observe that B1 and B2 have two copies of the filter

I see what you mean now, and I observe that behavior too. In that case, since there is a need for unique OBSSource items in the collection, using remove_items as I mentioned above would only work if you went to the trouble of converting it to a set or applying sort and unique.

bershanskiy · 2021-03-12T22:51:14Z

I see what you mean now, and I observe that behavior too. In that case, since there is a need for unique OBSSource items in the collection, using remove_items as I mentioned above would only work if you went to the trouble of converting it to a set or applying sort and unique.

I recently pushed an updated commit which does two things:

it uses std::set<OBSSource> to de-duplicate sources
it adds a simple confirmation prompt

bershanskiy · 2021-03-23T11:49:25Z

May I do anything to advance this PR?

bershanskiy · 2021-04-20T11:34:42Z

Converting this to draft because I think undo-redo system has a bug #4562 which affects this PR.

WizardCM · 2021-08-21T02:53:55Z

As the linked issues are fixed, would you be able to rebase this PR, clang format it, and then verify if all concerns have been dealt with so that it can be undrafted? Thanks!

bershanskiy · 2021-09-06T09:29:15Z

I rebased this PR and tested manually and it works as expected.

bershanskiy · 2021-09-18T12:44:09Z

Should I do anything to advance this PR?

WizardCM · 2022-02-27T00:55:00Z

Hi, there are merge conflicts. Once the conflicts are resolved, we will perform a final review and/or merge.

PatTheMav · 2023-12-05T18:46:17Z

Closing as last call for rebasing this PR for proper review went unanswered for now over a year and a half.

Can be reopened if a rebase took place and code is ready to be reviewed.

WizardCM added the kind/bug Categorizes issue or PR as related to a bug. label Mar 11, 2021

bershanskiy force-pushed the 4290 branch 2 times, most recently from 1ad30f2 to 64d2619 Compare March 12, 2021 22:38

WizardCM requested changes Mar 12, 2021

View reviewed changes

Comment thread UI/window-basic-main.cpp Outdated

Comment thread UI/window-basic-main.cpp Outdated

RytoEX reviewed Mar 12, 2021

View reviewed changes

Comment thread UI/window-basic-main.cpp Outdated

bershanskiy force-pushed the 4290 branch 2 times, most recently from 8bcb698 to 22e36be Compare March 12, 2021 23:37

WizardCM requested changes Mar 13, 2021

View reviewed changes

Comment thread UI/data/locale/en-US.ini Outdated

bershanskiy force-pushed the 4290 branch from 22e36be to 826256b Compare March 13, 2021 07:30

WizardCM approved these changes Mar 13, 2021

View reviewed changes

bershanskiy marked this pull request as draft April 20, 2021 11:33

bershanskiy mentioned this pull request Apr 20, 2021

Undo-redo feature does not work with copy-pasting filters (two scenarios) #4562

Closed

bershanskiy force-pushed the 4290 branch from 60ee9e0 to d2c12da Compare April 30, 2021 20:49

bershanskiy force-pushed the 4290 branch from d2c12da to c64ee56 Compare September 2, 2021 14:56

UI: Add Undo/Redo for pasting filters to multiple sources

945c4f4

bershanskiy force-pushed the 4290 branch from c64ee56 to 945c4f4 Compare September 2, 2021 15:13

bershanskiy marked this pull request as ready for review September 6, 2021 09:28

PatTheMav closed this Dec 5, 2023

Uh oh!

Conversation

bershanskiy commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

WizardCM commented Mar 11, 2021

Uh oh!

RytoEX commented Mar 12, 2021

Uh oh!

bershanskiy commented Mar 12, 2021

Uh oh!

RytoEX commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RytoEX commented Mar 12, 2021

Uh oh!

bershanskiy commented Mar 12, 2021

Uh oh!

RytoEX commented Mar 12, 2021

Uh oh!

bershanskiy commented Mar 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bershanskiy commented Mar 23, 2021

Uh oh!

bershanskiy commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WizardCM commented Aug 21, 2021

Uh oh!

bershanskiy commented Sep 6, 2021

Uh oh!

bershanskiy commented Sep 18, 2021

Uh oh!

WizardCM commented Feb 27, 2022

Uh oh!

PatTheMav commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bershanskiy commented Mar 11, 2021 •

edited

Loading

RytoEX commented Mar 12, 2021 •

edited

Loading

bershanskiy commented Apr 20, 2021 •

edited

Loading