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

refactor: remove arguments from dialog fragment constructors #4684

Merged
merged 8 commits into from
Sep 8, 2023

Conversation

anshtya
Copy link
Contributor

@anshtya anshtya commented Sep 3, 2023

Addresses #4434

@anshtya anshtya changed the title Fixes #4434 Fixes issue #4434 Sep 3, 2023
@Bnyro Bnyro changed the title Fixes issue #4434 refactor: remove arguments from dialog fragment constructors Sep 3, 2023
@Bnyro
Copy link
Member

Bnyro commented Sep 3, 2023

The same would probably also need to be done for everything in .ui.sheets, i.e. all bottom sheets since they're dialogs as well.

@Bnyro
Copy link
Member

Bnyro commented Sep 3, 2023

Please save all the keys used for the bundles inside the IntentData object as a constant so that we can avoid typos in the key and keep the code cleaner.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 3, 2023

Please save all the keys used for the bundles inside the IntentData object as a constant so that we can avoid typos in the key and keep the code cleaner.

Ok. Should I commit from here directly by clicking on 'Commit Suggestion' button or by pushing the changes to the branch?

@Bnyro
Copy link
Member

Bnyro commented Sep 3, 2023

It would be better if you do the suggested changes on your PC and push it via git as usual.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 3, 2023

It would be better if you do the suggested changes on your PC and push it via git as usual.

Ok.

@Bnyro
Copy link
Member

Bnyro commented Sep 3, 2023

Thanks for your work on that and applying the suggestions, looks fine to merge from my side when we can get the FragmentResultListener to work.

@Bnyro
Copy link
Member

Bnyro commented Sep 3, 2023

If you want to have a look at the bottom sheets as well: we sometimes pass the player there, the easiest thing probably would be to save the player in the PlayerViewModel and use it from there.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 4, 2023

If you want to have a look at the bottom sheets as well: we sometimes pass the player there, the easiest thing probably would be to save the player in the PlayerViewModel and use it from there.

Ok.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 4, 2023

A couple of things I want to tell:

  1. I have removed the arguments from the remaining dialog's constrcutors except the dialogs involving PlaylistOptionsBottomSheet due to some problem with FragmentResultListener and the ColorPickerDialog (2nd point). I'll push the changes later.

  2. In ColorPickerDialog there an argument for Context. I am not sure how to pass it through Bundle. I will think of its workaround later.

  3. There is conflict in app/src/main/java/com/github/libretube/ui/preferences/MainSettings.kt due to the recent push in master. I am not sure what to do for it. Resolve Conflicts button is appearing for me in this PR.

@Bnyro
Copy link
Member

Bnyro commented Sep 4, 2023

  1. Sounds good 👍
  2. We can't pass the context like that, however bottom sheets should have its own context after onCreate was called. So just make everything that is initialized with the context lateinit var and create it in the onCreate function.
  3. No worries about that, I can take care of it when merging.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 4, 2023

Why status checks aren't running ?

@Bnyro
Copy link
Member

Bnyro commented Sep 4, 2023

Probably due to the merge conflicts.
I don't have my PC with me currently, but in theory you can just click the resolve conflicts button and remove the two or three lines indicating that there's a conflict (it should be fine to just keep all the code as it is there).

@anshtya
Copy link
Contributor Author

anshtya commented Sep 5, 2023

  1. Sounds good 👍
  2. We can't pass the context like that, however bottom sheets should have its own context after onCreate was called. So just make everything that is initialized with the context lateinit var and create it in the onCreate function.
  3. No worries about that, I can take care of it when merging.
  1. ColorPickerDialog is working as expected without supplying the context. It only uses to context for showing toast messages.
  2. Could you please check the MainSettings.kt that I have pushed?

Everything is done. If there aren't any things need to be corrected from your side, I will push the remaining Dialogs.

@Bnyro
Copy link
Member

Bnyro commented Sep 5, 2023

MainSettings.kt looks good to me 👍

@anshtya
Copy link
Contributor Author

anshtya commented Sep 5, 2023

Is there something wrong with IntentData.kt, code quality check is failing.

@Bnyro
Copy link
Member

Bnyro commented Sep 5, 2023

There's a package called ktlint for Linux, with that one installed you can run ktlint --code-style=android-studio -F to automatically format the code.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 5, 2023

There's a package called ktlint for Linux, with that one installed you can run ktlint --code-style=android-studio -F to automatically format the code.

Ktlint doesn't show anything wrong in IntentData for me. I use windows.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 6, 2023

There's a package called ktlint for Linux, with that one installed you can run ktlint --code-style=android-studio -F to automatically format the code.

I have tried placing the IntentData in a new Android Project with ktlint setup and it. After doing the ktlint format command the code in IntentData remains same and doesn't give any errors.

@Bnyro
Copy link
Member

Bnyro commented Sep 6, 2023

It asks us to use variable names like PLAYLIST_DESCRIPTION instead of playlistDescription, but we can safely ignore this.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 6, 2023

If everything works from your perspective, we can unmark it as draft. However, I'd like to test this a bit myself (and others as well) first before merging to avoid any new issues by the changes.

Sure. Just send me a reply for any changes or to unmark from draft after you have tested.

@Bnyro
Copy link
Member

Bnyro commented Sep 6, 2023

  1. When renaming a playlist or changing its description it's not visible in the UI immediately as it used to, the playlist has to be reloaded which we should fix before merging.
  2. When changing the theme mode, all crashes while a dialog is opened seem to be gone (which is great). However, we still need to do the same thing for all bottom sheets to fix the issue, but that can be done by someone else in a new PR.

If 1. is fixed, it's okay to be merged from my POV.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 6, 2023

  1. When renaming a playlist or changing its description it's not visible in the UI immediately as it used to, the playlist has to be reloaded which we should fix before merging.
  2. When changing the theme mode, all crashes while a dialog is opened seem to be gone (which is great). However, we still need to do the same thing for all bottom sheets to fix the issue, but that can be done by someone else in a new PR.

If 1. is fixed, it's okay to be merged from my POV.

  1. Please recheck. It is updated immediately. Your renaming code isn't changed by me. Only thing that is changed is where the code is called.

@Bnyro
Copy link
Member

Bnyro commented Sep 6, 2023

No, that's clearly not working as it does before the changes.

@Bnyro
Copy link
Member

Bnyro commented Sep 6, 2023

We can consider passing the callback functions differently instead of using the FragmentResultListener:

val renamePlaylistDialog = RenamePlaylistDialog()
renamePlaylistDialog.onRename = {
    // insert the callback function content here
}
renamePlaylistDialog.show(fragmentManager, "foo")

where onRename is a public variable of type () -> Unit.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 6, 2023

We can consider passing the callback functions differently instead of using the FragmentResultListener:

val renamePlaylistDialog = RenamePlaylistDialog()
renamePlaylistDialog.onRename = {
    // insert the callback function content here
}
renamePlaylistDialog.show(fragmentManager, "foo")

where onRename is a public variable of type () -> Unit.

Ok.

@anshtya
Copy link
Contributor Author

anshtya commented Sep 6, 2023

We can consider passing the callback functions differently instead of using the FragmentResultListener:

val renamePlaylistDialog = RenamePlaylistDialog()
renamePlaylistDialog.onRename = {
    // insert the callback function content here
}
renamePlaylistDialog.show(fragmentManager, "foo")

where onRename is a public variable of type () -> Unit.

Done.

@anshtya anshtya marked this pull request as ready for review September 6, 2023 16:00
@anshtya
Copy link
Contributor Author

anshtya commented Sep 7, 2023

  1. When renaming a playlist or changing its description it's not visible in the UI immediately as it used to, the playlist has to be reloaded which we should fix before merging.
  2. When changing the theme mode, all crashes while a dialog is opened seem to be gone (which is great). However, we still need to do the same thing for all bottom sheets to fix the issue, but that can be done by someone else in a new PR.

If 1. is fixed, it's okay to be merged from my POV.

Any other changes ?

@Bnyro Bnyro force-pushed the fix/issue-4434 branch 3 times, most recently from 4560a80 to 061eecf Compare September 8, 2023 13:18
- ColorPickerDialog
- DeletePlaylistDialog
- RenamePlaylistDialog
Copy link
Member

@Bnyro Bnyro left a comment

Choose a reason for hiding this comment

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

Thank you!

@Bnyro Bnyro merged commit 5df822e into libre-tube:master Sep 8, 2023
2 of 3 checks passed
@Bnyro
Copy link
Member

Bnyro commented Sep 8, 2023

Are you interested in looking into the bottom sheets too, or shall I do that?

@anshtya
Copy link
Contributor Author

anshtya commented Sep 8, 2023

Are you interested in looking into the bottom sheets too, or shall I do that?

I would but I am kinda busy nowadays due to college. Thanks.

@Bnyro
Copy link
Member

Bnyro commented Sep 8, 2023

We're not in a hurry, you can do it whenever you want :)

@anshtya
Copy link
Contributor Author

anshtya commented Sep 8, 2023

We're not in a hurry, you can do it whenever you want :)

Ok. Sounds good.

@anshtya anshtya deleted the fix/issue-4434 branch September 8, 2023 16:19
@Bnyro
Copy link
Member

Bnyro commented Sep 14, 2023

For your information, I've already done the changes in some separate PRs now.

Nevertheless, feel free to work on any other issue whenever you want to :)

@anshtya
Copy link
Contributor Author

anshtya commented Sep 14, 2023

For your information, I've already done the changes in some separate PRs now.

Nevertheless, feel free to work on any other issue whenever you want to :)

Ok 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants