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

Save reply info in draft, refactor #449

Merged
merged 2 commits into from Nov 16, 2017

Conversation

Projects
None yet
2 participants
@charlag
Collaborator

charlag commented Nov 12, 2017

This fixes #417 and closes #440 and there's also a little bit of refactoring.
I actually did much more than I should have done first. I've tried saving Mentions but turns out I didn't need them for this. It would be a good idea to save the toot we're replying to but the DB is not ready yet for such thing. So I've just added some fields and that's it.
I didn't understand the visibility logic first. I hope I didn't break anything.
I've tried to remove as much as possible from ComposeActivity because it's huge.
Sorry that it's all smashed together but it's mostly related things.

@charlag charlag requested a review from connyduck Nov 12, 2017

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 12, 2017

Member

Wow thx for the work!

Unfortunately, this crashed when opening a saved status that was a reply

java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.equals(java.lang.Object)' on a null object reference
                                                                             at com.keylesspalace.tusky.ComposeActivity.onCreate(ComposeActivity.java:295)
                                                                             at android.app.Activity.performCreate(Activity.java:6975)
                                                                             at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1213)
                                                                             at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2770)
                                                                             at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2892) 
                                                                             at android.app.ActivityThread.-wrap11(Unknown Source:0) 
                                                                             at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1593) 
                                                                             at android.os.Handler.dispatchMessage(Handler.java:105) 
                                                                             at android.os.Looper.loop(Looper.java:164) 
                                                                             at android.app.ActivityThread.main(ActivityThread.java:6541) 
                                                                             at java.lang.reflect.Method.invoke(Native Method) 
                                                                             at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240) 
                                                                             at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
Member

connyduck commented Nov 12, 2017

Wow thx for the work!

Unfortunately, this crashed when opening a saved status that was a reply

java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.equals(java.lang.Object)' on a null object reference
                                                                             at com.keylesspalace.tusky.ComposeActivity.onCreate(ComposeActivity.java:295)
                                                                             at android.app.Activity.performCreate(Activity.java:6975)
                                                                             at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1213)
                                                                             at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2770)
                                                                             at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2892) 
                                                                             at android.app.ActivityThread.-wrap11(Unknown Source:0) 
                                                                             at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1593) 
                                                                             at android.os.Handler.dispatchMessage(Handler.java:105) 
                                                                             at android.os.Looper.loop(Looper.java:164) 
                                                                             at android.app.ActivityThread.main(ActivityThread.java:6541) 
                                                                             at java.lang.reflect.Method.invoke(Native Method) 
                                                                             at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240) 
                                                                             at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 12, 2017

Collaborator

Ah, I fixed it and then rolled back. Weird thing, two branches of if are actually doing the same thing.

Collaborator

charlag commented Nov 12, 2017

Ah, I fixed it and then rolled back. Weird thing, two branches of if are actually doing the same thing.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 12, 2017

Collaborator

I hope it's fixed now.
I won't be surprised if there are other bugs as well, changes are pretty big and I didn't really test migrations (Room even has tools for that).

Collaborator

charlag commented Nov 12, 2017

I hope it's fixed now.
I won't be surprised if there are other bugs as well, changes are pretty big and I didn't really test migrations (Room even has tools for that).

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

Well I tested a bit.

If you try replying to a status that has been deleted, server return 404. We just show "an error occured". I think that should be improved, users will not know what to do.

but the big problem is this:

java.lang.IllegalStateException: Migration didn't properly handle TootEntity(com.keylesspalace.tusky.db.TootEntity).
                                                                          Expected:
                                                                         TableInfo{name='TootEntity', columns={uid=Column{name='uid', type='INTEGER', notNull=true, primaryKeyPosition=1}, contentWarning=Column{name='contentWarning', type='TEXT', notNull=false, primaryKeyPosition=0}, urls=Column{name='urls', type='TEXT', notNull=false, primaryKeyPosition=0}, visibility=Column{name='visibility', type='INTEGER', notNull=false, primaryKeyPosition=0}, inReplyToText=Column{name='inReplyToText', type='TEXT', notNull=false, primaryKeyPosition=0}, text=Column{name='text', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToUsername=Column{name='inReplyToUsername', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToId=Column{name='inReplyToId', type='TEXT', notNull=false, primaryKeyPosition=0}}, foreignKeys=[], indices=[]}
                                                                          Found:
                                                                         TableInfo{name='TootEntity', columns={uid=Column{name='uid', type='INTEGER', notNull=true, primaryKeyPosition=1}, contentWarning=Column{name='contentWarning', type='TEXT', notNull=false, primaryKeyPosition=0}, urls=Column{name='urls', type='TEXT', notNull=false, primaryKeyPosition=0}, visibility=Column{name='visibility', type='INTEGER', notNull=false, primaryKeyPosition=0}, inReplyToText=Column{name='inReplyToText', type='STRING', notNull=false, primaryKeyPosition=0}, text=Column{name='text', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToUsername=Column{name='inReplyToUsername', type='STRING', notNull=false, primaryKeyPosition=0}, inReplyToId=Column{name='inReplyToId', type='STRING', notNull=false, primaryKeyPosition=0}}, foreignKeys=[], indices=[]}
                                                                             at com.keylesspalace.tusky.db.AppDatabase_Impl$1.validateMigration(AppDatabase_Impl.java:69)
                                                                             at android.arch.persistence.room.RoomOpenHelper.onUpgrade(RoomOpenHelper.java:75)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onUpgrade(FrameworkSQLiteOpenHelper.java:118)
                                                                             at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:299)
                                                                             at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:194)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableSupportDatabase(FrameworkSQLiteOpenHelper.java:93)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.java:54)
                                                                             at android.arch.persistence.room.RoomDatabase.query(RoomDatabase.java:193)
                                                                             at com.keylesspalace.tusky.db.TootDao_Impl.loadAll(TootDao_Impl.java:110)
                                                                             at com.keylesspalace.tusky.SavedTootActivity$FetchPojosTask.doInBackground(SavedTootActivity.java:171)
                                                                             at com.keylesspalace.tusky.SavedTootActivity$FetchPojosTask.doInBackground(SavedTootActivity.java:161)
                                                                             at android.os.AsyncTask$2.call(AsyncTask.java:333)
                                                                             at java.util.concurrent.FutureTask.run(FutureTask.java:266)

happens when you go to the saved toots activity after upgrading master to your branch.

If I read it correctly, colums have the wrong format (there seem to be "STRING" and "TEXT" column types)

Member

connyduck commented Nov 13, 2017

Well I tested a bit.

If you try replying to a status that has been deleted, server return 404. We just show "an error occured". I think that should be improved, users will not know what to do.

but the big problem is this:

java.lang.IllegalStateException: Migration didn't properly handle TootEntity(com.keylesspalace.tusky.db.TootEntity).
                                                                          Expected:
                                                                         TableInfo{name='TootEntity', columns={uid=Column{name='uid', type='INTEGER', notNull=true, primaryKeyPosition=1}, contentWarning=Column{name='contentWarning', type='TEXT', notNull=false, primaryKeyPosition=0}, urls=Column{name='urls', type='TEXT', notNull=false, primaryKeyPosition=0}, visibility=Column{name='visibility', type='INTEGER', notNull=false, primaryKeyPosition=0}, inReplyToText=Column{name='inReplyToText', type='TEXT', notNull=false, primaryKeyPosition=0}, text=Column{name='text', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToUsername=Column{name='inReplyToUsername', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToId=Column{name='inReplyToId', type='TEXT', notNull=false, primaryKeyPosition=0}}, foreignKeys=[], indices=[]}
                                                                          Found:
                                                                         TableInfo{name='TootEntity', columns={uid=Column{name='uid', type='INTEGER', notNull=true, primaryKeyPosition=1}, contentWarning=Column{name='contentWarning', type='TEXT', notNull=false, primaryKeyPosition=0}, urls=Column{name='urls', type='TEXT', notNull=false, primaryKeyPosition=0}, visibility=Column{name='visibility', type='INTEGER', notNull=false, primaryKeyPosition=0}, inReplyToText=Column{name='inReplyToText', type='STRING', notNull=false, primaryKeyPosition=0}, text=Column{name='text', type='TEXT', notNull=false, primaryKeyPosition=0}, inReplyToUsername=Column{name='inReplyToUsername', type='STRING', notNull=false, primaryKeyPosition=0}, inReplyToId=Column{name='inReplyToId', type='STRING', notNull=false, primaryKeyPosition=0}}, foreignKeys=[], indices=[]}
                                                                             at com.keylesspalace.tusky.db.AppDatabase_Impl$1.validateMigration(AppDatabase_Impl.java:69)
                                                                             at android.arch.persistence.room.RoomOpenHelper.onUpgrade(RoomOpenHelper.java:75)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.onUpgrade(FrameworkSQLiteOpenHelper.java:118)
                                                                             at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:299)
                                                                             at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:194)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper$OpenHelper.getWritableSupportDatabase(FrameworkSQLiteOpenHelper.java:93)
                                                                             at android.arch.persistence.db.framework.FrameworkSQLiteOpenHelper.getWritableDatabase(FrameworkSQLiteOpenHelper.java:54)
                                                                             at android.arch.persistence.room.RoomDatabase.query(RoomDatabase.java:193)
                                                                             at com.keylesspalace.tusky.db.TootDao_Impl.loadAll(TootDao_Impl.java:110)
                                                                             at com.keylesspalace.tusky.SavedTootActivity$FetchPojosTask.doInBackground(SavedTootActivity.java:171)
                                                                             at com.keylesspalace.tusky.SavedTootActivity$FetchPojosTask.doInBackground(SavedTootActivity.java:161)
                                                                             at android.os.AsyncTask$2.call(AsyncTask.java:333)
                                                                             at java.util.concurrent.FutureTask.run(FutureTask.java:266)

happens when you go to the saved toots activity after upgrading master to your branch.

If I read it correctly, colums have the wrong format (there seem to be "STRING" and "TEXT" column types)

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

@connyduck thanks for testing it properly! Will fix is as soon as I can.

Collaborator

charlag commented Nov 13, 2017

@connyduck thanks for testing it properly! Will fix is as soon as I can.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

If think I might make a request to check if the reply is still available but I am not sure. Should I just make a special case for 400 error with message telling that it may be related to the reply? I could show a dialog box/snackbar asking if we should delete the reply and post it like a regular status.

Collaborator

charlag commented Nov 13, 2017

If think I might make a request to check if the reply is still available but I am not sure. Should I just make a special case for 400 error with message telling that it may be related to the reply? I could show a dialog box/snackbar asking if we should delete the reply and post it like a regular status.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

Request to check would be great, but you still got the problem that the original toot could have been deleted between checking and sending the reply. We should check in the server code if the 404 status code reliably means that the original toot got deleted.

Member

connyduck commented Nov 13, 2017

Request to check would be great, but you still got the problem that the original toot could have been deleted between checking and sending the reply. We should check in the server code if the 404 status code reliably means that the original toot got deleted.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

@connyduck good spot! I will make check for error code then.

Collaborator

charlag commented Nov 13, 2017

@connyduck good spot! I will make check for error code then.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

Sorry, I do not know Ruby well enough to verify it.

Member

connyduck commented Nov 13, 2017

Sorry, I do not know Ruby well enough to verify it.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

@connyduck excuse me?

Collaborator

charlag commented Nov 13, 2017

@connyduck excuse me?

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

I've fixed (and tested) migration and I've also added code to handle the 404 case.

Collaborator

charlag commented Nov 13, 2017

I've fixed (and tested) migration and I've also added code to handle the 404 case.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

I am not sure 404 can only appear when parent toot is missing, but better than before 👍

Member

connyduck commented Nov 13, 2017

I am not sure 404 can only appear when parent toot is missing, but better than before 👍

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 13, 2017

Collaborator

@connyduck well, that's what that user dialog is for. We're saying "you know, sometimes it may be because of the missing toot". I'm not sure how to handle this better.

Collaborator

charlag commented Nov 13, 2017

@connyduck well, that's what that user dialog is for. We're saying "you know, sometimes it may be because of the missing toot". I'm not sure how to handle this better.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

Ok one more thing. It is possible to save the same toot a hundred times. Can you add a boolean that tracks if a toot has been changed since last save and only save it when it has changed?

Member

connyduck commented Nov 13, 2017

Ok one more thing. It is possible to save the same toot a hundred times. Can you add a boolean that tracks if a toot has been changed since last save and only save it when it has changed?

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 13, 2017

Member

Or make the combination of inReplyToId, contentWarning, urls and text unique in the db.

Member

connyduck commented Nov 13, 2017

Or make the combination of inReplyToId, contentWarning, urls and text unique in the db.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 14, 2017

Collaborator

@connyduck I'm not sure if it's what you've asked for but now it will not make many separate records but will rather update existing one.

Collaborator

charlag commented Nov 14, 2017

@connyduck I'm not sure if it's what you've asked for but now it will not make many separate records but will rather update existing one.

@charlag charlag referenced this pull request Nov 14, 2017

Closed

[WIP] Multiaccount #459

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 15, 2017

Member

Oh no thats not what i meant & I am not sure if it is that easy, but we can do without it for now.
If you undo the last commit b2dad97 this is ready to merge for me!

Member

connyduck commented Nov 15, 2017

Oh no thats not what i meant & I am not sure if it is that easy, but we can do without it for now.
If you undo the last commit b2dad97 this is ready to merge for me!

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 15, 2017

Collaborator

Oh, sorry! This commits prevents creation of many drafts in the situation where one should be updated. Are you sure we don't need that? I'm not sure I understand the reasoning.

Collaborator

charlag commented Nov 15, 2017

Oh, sorry! This commits prevents creation of many drafts in the situation where one should be updated. Are you sure we don't need that? I'm not sure I understand the reasoning.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Nov 15, 2017

Member

Well, on Tusky 1.3, you can compose a toot, save it, edit it and save another copy. This way you can save multiple Toots without leaving ComposeActivity. Thats not possible anymore with your change. I would just prohibit saving EXACT duplicates of toots.

Member

connyduck commented Nov 15, 2017

Well, on Tusky 1.3, you can compose a toot, save it, edit it and save another copy. This way you can save multiple Toots without leaving ComposeActivity. Thats not possible anymore with your change. I would just prohibit saving EXACT duplicates of toots.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 15, 2017

Collaborator

Ah, I see, you expect a different behaviour model than I do. I can check for exact same posts, no worries, but it will take some time and I think it should be in another PR. I will remove this commit then.

Collaborator

charlag commented Nov 15, 2017

Ah, I see, you expect a different behaviour model than I do. I can check for exact same posts, no worries, but it will take some time and I think it should be in another PR. I will remove this commit then.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Nov 15, 2017

Collaborator

I've removed last commit and rebased it on master to fix merge conflicts.

Collaborator

charlag commented Nov 15, 2017

I've removed last commit and rebased it on master to fix merge conflicts.

@connyduck connyduck merged commit 2575b16 into tuskyapp:master Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment