ReVanced
feat: Reuse patch selector screen to view patches from a bundle
Feature description
Use this screen:
for this:
It would be necessary, to add to the ViewModel to make this screen "readonly" conditionally. Checkboxes indicate the default use state of the patches, so they can be made readonly, the buttons need to disappear, in the options the edit button icon needs to change and the dialogs need to be readonly without a button "Save" and instead "Close". What is currently missing from that screen is the packages and versions it is compatible with. This needs to be added when this screen is used for view patches.
Motivation
The initial idea was to match the design of website. However this adds stylistic inconsistencies with the rest of the app. It is better to remain consistent with itself than with an independent project like website. At the same time we reuse components and reduce code.
Acknowledgements
- [x] I have checked all open and closed feature requests and this is not a duplicate.
- [x] I have chosen an appropriate title.
- [x] The feature request is only related to ReVanced Manager.
The patch selector screen and bundle patches screen are very different internally, since one of them has a lot of state, needs to handle multiple bundles and previous selection. The patch selector also only shows patches for a specific package while the bundle selector needs to show patches for all packages in the specific bundle. In this case I believe the cost of adding several branches to handle different screens is higher than keeping them separate. It does probably make sense to reuse code for UI components though.
The only difference between the patch selector screen and the patch view screen is that one is selectable and its options can be edited. Thats two branches. Another is to hide the patch button. Sounds straight forward to me and reduces code and increases consistency by getting rid of one entire view and reusing an existing one. The patch selector can show compatible versions and packages as well just fine, however if that is not desired, then an if branch once again takes care of that just fine
Note, if its two views, making changes adds tech debt because you need to consider the other view as well, however feel free to execute this issue however you prefer. The main point is to fix consistency
I am going to remove this from the merge to main milestone unless a motivation as to why this needs to be on that milestone is provided.
Regarding code reuse, I believe it makes more sense to reuse individual components rather than the entire view model or screen. That way, the screens/VMs can deal with the things specific to them without worrying about the other screen.
I am not sure I understand your reasoning, but the difference is merely mutability. So that just needs to be parametrized in the model
I still believe this should be kept in the milestone due to its low complexity but strong consistency return
I am not sure I understand your reasoning, but the difference is merely mutability. So that just needs to be parametrized in the model
My assessment is based on this comment and my knowledge of the codebase. I can give you links to the code for the relevant screens if you want.
I still believe this should be kept in the milestone due to its low complexity but strong consistency return
Implementation compexity is easy to underestimate and implementing 100 simple issues still takes significant effort. Therefore, apparent complexity should not justify a place on that milestone. If you are referring to code consistency then I don't think it justifies being on the milestone either, since refactoring can always be done after merge to main and does not directly affect users. If you are referring to UI consistency, can you show a specific example and motivate why it is bad and justifies milestone placement?
My assessment is based on this comment
Guess what can change is viewing per patches then. Tapping on view patches for a specific patches file can open the patch selector in read only on the respective patches tab. The bundle page can technically be replaced entirely as well by moving the details into the patch selector tabs page. At the top the patches details would be shown, scrolling down would show the list like it does now. This would render an entire view useless, restore consistency and make the codebase simpler at the same time. Managing state doesn't sound blocking. You can keep the state management code but since Viewing is read only, it would simply have one activate state.
Therefore, apparent complexity should not justify a place on that milestone.
I have done parts of this issues work already when I created the issue to confirm if it's feasible, which it was. I got rid of it though since I didn't finish it, but it took me approximately 10 minutes to almost handle everything. It was merely adding if blocks everywhere where edits could be made by a user
If you are referring to code consistency then I don't think it justifies being on the milestone either, since refactoring can always be done after merge to main and does not directly affect users.
It's a supporting argument non the less. You swat multiple flies like this, at the same time make the ui more consistent for the user
If you are referring to UI consistency, can you show a specific example and motivate why it is bad and justifies milestone placement?
Well I am not sure what you mean by example. You have two views that do the same thing except for what makes them different. Ye the code and ui does not reflect this property
Guess what can change is viewing per patches then. Tapping on view patches for a specific patches file can open the patch selector in read only on the respective patches tab. The bundle page can technically be replaced entirely as well by moving the details into the patch selector tabs page. At the top the patches details would be shown, scrolling down would show the list like it does now. This would render an entire view useless, restore consistency and make the codebase simpler at the same time. Managing state doesn't sound blocking. You can keep the state management code but since Viewing is read only, it would simply have one activate state.
This would mean you have to select an app in order to view patches for it. Regardless, big design changes are up to the designers, not you and me.
Therefore, apparent complexity should not justify a place on that milestone.
I have done parts of this issues work already when I created the issue to confirm if it's feasible, which it was. I got rid of it though since I didn't finish it, but it took me approximately 10 minutes to almost handle everything. It was merely adding if blocks everywhere where edits could be made by a user
This was never about whether the issue was feasible or not. You never refuted my argument that issues shouldn't be on the milestone just because they seem simple.
If you are referring to code consistency then I don't think it justifies being on the milestone either, since refactoring can always be done after merge to main and does not directly affect users.
It's a supporting argument non the less. You swat multiple flies like this, at the same time make the ui more consistent for the user
Sharing code does not guarantee the end result will be good. I remember splitting a viewmodel which was used for the different update screens (settings, update download and changelog I think) into 3 smaller, higher quality viewmodels and the result was much better.
This would mean you have to select an app in order to view patches for it. Regardless, big design changes are up to the designers, not you and me.
No app needs to be selected. The view patches button would open the view with the model including the patches of the selected patches.
This was never about whether the issue was feasible or not. You never refuted my argument that issues shouldn't be on the milestone just because they seem simple.
It should be, because it's the biggest inconsistency in the app. Therefore lacks features like sorting and searching. I didn't mention this because I thought it's implicit enough that it doesn't need explicit mention
Sharing code does not guarantee the end result will be good. I remember splitting a viewmodel which was used for the different update screens (settings, update download and changelog I think) into 3 smaller, higher quality viewmodels and the result was much better.
In this case it is good. We are viewing patches. This is semantically and structurally equal to selecting patches but just immutable. It's perfect candidate to be shared. You simply open the view with a model configured to read only. In this case you can simply use the same logic as selecting patches without the app filter, read only and the selected patch to view to select on launch. This way the view patches would show the selected patches and users on that view can directly navigate to other patches without having to go back to the respective patches & you reuse existing code at the same time