added check for UI Thread #605
Conversation
|
Yes, |
|
Ok, I'll find a better way than using direct() |
|
I think I got something working well. I'm not sure what (if any) test cases need to be updated for this though. |
|
This change may break existing code in pretty bad ways if the code relies on the current behaviour of always posting to the handler. Maybe you could introduce a flag to allow reusing like e.g. @UiThread(reuse = true)
void runOnMainThread() {
} |
|
@adrianroos If it was the other way around, I would agree immediately: a synchronous call becoming asynchronous can really break things. But can you think of a usecase in which a code relies on the fact that the call is asynchronous in the UI thread ? I can't. I could if it was on a background thread (download things or doing any IO stuff) but on the main thread, I can't. Please tell me if I'm wrong. |
|
@JoanZapata posting to a Handler is not just asynchronous, if you do it on the same thread it guarantees that the execution will only happen once your current call stack has returned to the looper. For example, suppose we have some kind of listener registry: Set<Listener> listeners;
void register(Listener l) {
listeners.add(l);
}
void unregister(Listener l) {
listeners.remove(l);
}
void notifyListeners() {
for (Listener l : listeners) {
l.call();
}
}If you have a listener with this implementation: @UiThread
void call() {
registry.unregister(this);
}Then currently this will work, because the listener only unregisters itself after the iteration over all listeners has finnished. After the change, there would be a ConcurrentModificationException. Or even simpler: @UIThread
void doSomeWorkWithoutBlockingUI() {
// ...
if(!done) {
doSomeWorkWithoutBlockingUI()
}
} |
|
Ok you're right, thanks. |
|
I agree too. But I'm not sure for the field's name. Any other idea ? |
|
How would this variable be used exactly and what would be the default if it's not there? I would think the default would be to check if it's in the UI thread already and skip the handler. If that's the case, maybe the variable would be alwaysUseHandler, or noCheck or something like that. |
|
Nop. As we don't want to break the API the default behavior is to always put the runnable into the handler without any check. |
|
Understood, I agree. The only thing about directCall is I think it may be misleading, at least for people who don't read javadocs. Maybe something like uiThreadCheck or something to that effect. I guess any name really works as long as people read the docs, which of course they should. |
|
I added a fix where it would fail to compile if the @UiThread method had parameters. I haven't done anything about the new directCall property though, until a final name is decided. |
|
Ok I had some time today so I went ahead and added useDirect, we can always change it if we don't like the name. |
|
@rockytriton I really think you should wait when a discussion is pending on a issue, except if you have an immediate need. :) @DayS I liked @UIThread
public void method1(){
method2();
}
@UIThread(reuse = true)
public void method2(){ }I agree it can be misleading if it's called from another UiThread method which is not annotated, but I think it's more appropriate than Another way that could make the code easier to read would be to use an enum: @UIThread(REQUIRES_NEW) // default
@UIThread(NESTED)
or
@UIThread(PROPAGATION_REQUIRES_NEW) // default
@UIThread(PROPAGATION_NESTED)
or
@UIThread(propagation = REQUIRES_NEW) // default
@UIThread(propagation = NESTED)
|
|
👍 for the enum. It also prepares the annotation for some future improvement And yes, don't make changes or pull request before a solution has been validated. If you're solution doesn't fit the needs you'll have to make another commit to revert you code... |
|
I think I like this one: @UIThread(propagation = REQUIRES_NEW) // default
@UIThread(propagation = REUSE)Any thought ? @DayS @pyricau @adrianroos @mathieuboniface |
|
I think |
|
Just to make a decision and go forward on this. I propose to use a
I think you can start to code :) |
|
👍 |
|
Hey, sorry I just figured I would throw some ideas out there in code while I had the chance before I went on vacation. I'm still new to github so I don't know the normal protocol or if some ways are better than others. So just to be clear, should I go ahead and make these changes in this code fork or is someone already working on it on another? |
|
You can make a new commit on your PR with this code for the @Retention(RetentionPolicy.CLASS)
@Target(ElementType.METHOD)
public @interface UiThread {
long delay() default 0;
Propagation propagation() default Propagation.ENQUEUE;
public enum Propagation {
ENQUEUE, REUSE
}
}And modify your processor to handle this. |
There was a problem hiding this comment.
I think you shouldn't clone the body of the method and you should use the method callSuperMethod from APTCodeModelHelper
8000
(here) instead.
Because we are not sure that the annotation will be processed if the content of the method changes.
There was a problem hiding this comment.
Thanks, I didn't know that method existed. I will replace it.
|
@yDelouis I changed it to no longer use the clone. One question though, should that class APTCodeModelHelper be a static utility class instead? I don't feel it makes sense to instantiate it. |
|
I don't know why APTCodeModel methods are not static. Maybe it is to be consistent with other helpers. |
|
I'll review your new changes after the droidcon :) |
There was a problem hiding this comment.
You shouldn't codeModel.ref(<a class>) because it messed up sometimes.
Have a look to EBeansHolder class instead.
There was a problem hiding this comment.
The EBeansHolder.refClass(Class) just calls codeModel.ref(). Should I be using EBeansHolder.refClass(String) instead and use the String name of the class? Do you know what situations that the ref() method doesn't work in? It works in my application at least. Just want to make sure I am using it correctly.
|
you should also add some unit tests in ThreadActivity |
…iThread added check for UI Thread
|
Thanks for the feature 👍 |
What do you think about this for issue #604?
I don't know how you feel about using JExpr.direct(). I was trying to do it without it but couldn't figure it out immediately. I can look into it more if you don't like using direct().