-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Added new resource file system #397
base: master
Are you sure you want to change the base?
Conversation
This PR builds correctly, here is the generated apk.
You must be logged in for the link to work. This is an automatic comment created by a Github Action |
@@ -0,0 +1,71 @@ | |||
package com.trianguloy.urlchecker.modules.companions.ResourceCatalogs; |
Check warning
Code scanning / Pmd (reported by Codacy)
Package name contains upper case characters Warning
@@ -0,0 +1,65 @@ | |||
package com.trianguloy.urlchecker.modules.companions.ResourceCatalogs; |
Check warning
Code scanning / Pmd (reported by Codacy)
Package name contains upper case characters Warning
@@ -0,0 +1,24 @@ | |||
package com.trianguloy.urlchecker.modules.companions.ResourceCatalogs; |
Check warning
Code scanning / Pmd (reported by Codacy)
Package name contains upper case characters Warning
@@ -0,0 +1,25 @@ | |||
package com.trianguloy.urlchecker.modules.companions.ResourceCatalogs; |
Check warning
Code scanning / Pmd (reported by Codacy)
Package name contains upper case characters Warning
@@ -0,0 +1,23 @@ | |||
package com.trianguloy.urlchecker.modules.companions.ResourceCatalogs; |
Check warning
Code scanning / Pmd (reported by Codacy)
Package name contains upper case characters Warning
// ------------------- ui ------------------- | ||
|
||
Button updateButton; | ||
TextView txt_check; |
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Warning
|
||
Button updateButton; | ||
TextView txt_check; | ||
TextView txt_update; |
Check warning
Code scanning / Pmd (reported by Codacy)
Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes. Warning
super.clear(); | ||
} | ||
|
||
public void _update(boolean background) { |
Check warning
Code scanning / Pmd (reported by Codacy)
The instance method name '_update' doesn't match '[a-z][a-zA-Z0-9]*' Warning
* Network operation, designed to be run in a background thread. | ||
* Returns the message to display to the user about the result | ||
*/ | ||
protected UpdateResult _updateResource() { |
Check warning
Code scanning / Pmd (reported by Codacy)
The instance method name '_updateResource' doesn't match '[a-z][a-zA-Z0-9]*' Warning
/** | ||
* Saves a new local file. Returns if it was up to date, updated or an error happened. | ||
*/ | ||
private UpdateResult _save(String rawContent) { |
Check warning
Code scanning / Pmd (reported by Codacy)
The instance method name '_save' doesn't match '[a-z][a-zA-Z0-9]*' Warning
Let's see, It's true I would like to extract the logic for the 'automatic update of a file-like asset', as that will be needed in other modules (even now it will help with the current ones, as you mentioned in the description) My main concern is that the pr is...big. Not by a lot, I've seen bigger, but still I wonder if all the logic and classes are needed. Maybe they are, but I still need to review everything, and right now I simply can't :( So, for now I think it's best to leave this. But don't delete it! If I come back to this feature I'll definitely take a look at this and try to adapt it. Thank you for your effort, much appreciated! Alternatively, maybe what you can try to do (if you want) is to extract the current logic. Just a cut/paste of code, moving things from a file to another, and only adding minimal 'glue logic' when needed. That could help with a later refactor, but since this code is a bit 'entangled' with other things, I don't think it will be easy either... |
All the logic is needed, I think, however, the classes, well that is up for debate. Because the objective was to remove duplicated code, I tried to (strictly) make the responsibilites not interfere with each other, and that led to this. I might have overused the interfaces, and I don't think we will ever need non-json files.
That's the thing, I didn't write much code for this PR. The code for the methods is copied from other places in the source code, and the variables are too, maybe with different names. I avoided refactoring directly so involved classes could work correctly until they were explicitly moved to the new system. What if I do it like this?
I honestly don't mind having the PR open, if you don't mind either, and if this is something that at some point in the future will need to be revisited, then I don't see any problem on finishing this now, and leaving it open until review. If you are ok with this let me know and I will keep working on it. Btw, off-topic, I have a small patch for the automations, it allows for both Strings or arrays of strings in the "regex" field. I felt I was cluttering the file with too many "Unshort X shortener service" and thought it might be useful. I know that technically I can make a regex that supports everything, but this is more accessible and easier to maintain in my opinion. If the decision to have only Strings wasn't made on purpose, do you want me to make a PR on this? |
Me neither! so let's keep it for revisiting later when possible.
That's up to you, as I said I do want to extract the functionality, but you already know me, I prefer small prs...but I'm aware it may be impossible in this case. Your 6-steps process looks good for me, although maybe instead of a comment for each code piece you can add a single one at the class (at least when everything in a class comes from a single source)
Like the patterns module? where you can either specify |
So, I was going to do a Privacy redirector module as per #7. But I noticed all the logic to download files is in
ClearUrlCatalog.java
, to avoid code duplication I got sidetracked working on this.This is the, 4th(? or 3rd, I don't remember) iteration, but the code is still untested, I don't want to spend more time than necessary in something that might not be merged, althought it should work as it is mainly refactoring.
Here is the class diagram to make things clearer at a glance. I have omitted certain things so it is easier to follow
The main thing to worry is the diagonal of abstract classes as each relies on the previous.
Here is an example to show the actual use.
If you like this change, let me know and I will keep working on the things that will need to be done in order for this to work:
Let me know what you think! And don't worry if you feel like all this should be scrapped, it is a huge change. Take all the time you need I don't want to put any pressure on you, even if that means waiting for other PRs that are currently open.