Skip to content
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

[GSoC'24] Refactor: Consolidate media paths into a single variable #16750

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

SanjaySargam
Copy link
Contributor

Purpose / Description

This PR introduces a refactor to consolidate the media paths into a single variable mediaPath for better code maintainability and clarity.

How Has This Been Tested?

Realme 9 ( Physical Device )

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@SanjaySargam SanjaySargam added Needs Review GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors labels Jul 17, 2024
Copy link
Member

@Arthur-Milchior Arthur-Milchior left a comment

Choose a reason for hiding this comment

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

I assume you tested it, and can still import deck with image and sound. That you can still edit images

I admit I'd be more confortable if I knew why we had two fields originally. I guess there must have been a reason, but I can't easily understand it.
In particular because the value of the path is sometime changed, and sometime reverted to its previous value. So I would fear this would create interaction. Except that I don't see how you could be dealing with image and sound simultaneously.


// For Audio type
var audioPath: String?
// Unified media path for media types ( image, video and audio )
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove the "unified". It seems to indicate that it's the result of a unification. Which is true, that's what you're doing in this commit. But that's not something the developer that will read this code have to know.
So, just:

Path of the folder containing media used by AnkiDroid.

I would not list all the media, because technically, it can be anything else. It can be font, it can be css files, javascript libraries, and I'm sure some advanced users created decks with more esoteric data.

@@ -249,32 +249,22 @@ class MultimediaEditFieldActivity :
@KotlinCleanup("rename: bChangeToText")
private fun done() {
var bChangeToText = false
if (field.type === EFieldType.IMAGE) {
if (field.imagePath == null) {
if (field.type === EFieldType.IMAGE || field.type === EFieldType.AUDIO_RECORDING && isAudioRecordingSaved) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate if you could add parenthesis. I never know which is the priority order, and would prefer not to have to learn it by hearth to be sure the change is correct

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

I am ok with this if Sanjay wants to use it somewhere but this will go under major refactoring once I am done with all multimedia fragments most likely remove it and other multimedia field class and introduce a better way to handle it

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Cheers

Truthfully: I'd hope that this is used for a purpose, as we're aiming to remove IField etc...

But the code change is good and @criticalAY is happy

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Jul 21, 2024
@Arthur-Milchior Arthur-Milchior added this pull request to the merge queue Jul 21, 2024
Merged via the queue into ankidroid:main with commit b9aacde Jul 21, 2024
8 checks passed
@github-actions github-actions bot removed the Needs Second Approval Has one approval, one more approval to merge label Jul 21, 2024
@github-actions github-actions bot added this to the 2.19 release milestone Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC Pull requests authored by a Google Summer of Code participant [Candidate/Selected], for GSoC mentors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants