Skip to content

MS-854 Reducing duplication in capture live feedback classes#1303

Merged
luhmirin-s merged 4 commits into
mainfrom
feature/MS-854-capture-code-refactor
Aug 25, 2025
Merged

MS-854 Reducing duplication in capture live feedback classes#1303
luhmirin-s merged 4 commits into
mainfrom
feature/MS-854-capture-code-refactor

Conversation

@luhmirin-s
Copy link
Copy Markdown
Contributor

JIRA ticket
Will be released in: 2025.3.0

Notable changes

  • While looking at the diffs between manual and auto capture, I realised that there are not that many changes. So I moved the auto-capture specific code into the regular LiveFeedback<> classes with minor branching.
  • Also did a small cleanup in the old code.

Testing guidance

  • Run manual and auto-capture test cases.

Additional work checklist

  • Effect on other features and security has been considered
  • Design document marked as "In development" (if applicable)
  • External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
  • Test cases in Testiny are up to date (or ticket created)
  • Other teams notified about the changes (if applicable)

@cla-bot cla-bot Bot added the ... label Aug 6, 2025
@luhmirin-s luhmirin-s requested review from a team, BurningAXE, TristramN, alex-vt, alexandr-simprints, meladRaouf and ybourgery and removed request for a team August 6, 2025 07:31
@luhmirin-s luhmirin-s marked this pull request as ready for review August 6, 2025 07:31
operator fun invoke(projectConfiguration: ProjectConfiguration): Boolean {
val isFeatureEnabled = projectConfiguration.experimental().faceAutoCaptureEnabled
val isOptionTurnedOnInSettings = preference.getBoolean(AUTO_CAPTURE_PREFERENCE_KEY, true)
return isFeatureEnabled && isOptionTurnedOnInSettings
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor performance issue that's also present in the previous implementation - if isOptionTurnedOnInSettings is extracted as a function and placed inside the return statement, it will never be read when isFeatureEnabled is false (thus sparing shared preferences loading, reading & parsing).

@luhmirin-s luhmirin-s force-pushed the feature/MS-854-capture-code-refactor branch from 627a35f to cacbd23 Compare August 7, 2025 11:54
* to speed things up this method creates multiple async jobs and run them all in parallel.
*
* Auto-capture generates a number of sample captures that typically significantly exceeds samplesToKeep.
* Thus the non-qualifying samples are excluded to avoid excessive event data.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't see this comment reflected in the function (neither here, nor in the old file)!?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I didn't look too closely at that comment when merging the files. I agree with you.

Copy link
Copy Markdown
Contributor

@BurningAXE BurningAXE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pay special attention to this during next release testing to ensure we didn't break anything. Would be nice if @alex-vt takes a look when back, even if PR is already merged.

@luhmirin-s luhmirin-s force-pushed the feature/MS-854-capture-code-refactor branch from cacbd23 to 616c827 Compare August 7, 2025 12:08
@luhmirin-s
Copy link
Copy Markdown
Contributor Author

@BurningAXE @alex-vt I will leave it open for now and re-assign the task to Oleksii. Feel free to fix it up and/or merge at any time.

@luhmirin-s luhmirin-s force-pushed the feature/MS-854-capture-code-refactor branch from 616c827 to d4cfbc1 Compare August 25, 2025 05:44
@sonarqubecloud
Copy link
Copy Markdown

@luhmirin-s luhmirin-s merged commit 8a6de84 into main Aug 25, 2025
22 of 23 checks passed
@luhmirin-s luhmirin-s deleted the feature/MS-854-capture-code-refactor branch August 25, 2025 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants