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.

Sign up for GitHub

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

Jump to bottom

Pin message video audio attachments #79

Merged
merged 5 commits into from
Mar 9, 2018
Merged

Pin message video audio attachments #79

merged 5 commits into from
Mar 9, 2018

Conversation

veken1199
Copy link
Collaborator

Pin full functionality for normal conversations and group chat

@cool-appz
Bug fix #58 and #63:
678dce9 
1- Removed the additional verification we placed last semester to block audio and video testing
2- Made sure that we can pin and unpin videos and audios in both 1-1 conversation and groupchat
@cool-appz
Related #58, #63, #62:
8a9b14b 
1- Added Audio stub object to pinned messages
2- Added pinned audio xml views
3- Introduced isAudio and isVideo to pinnedMessagesAdapter
4- Refactored the way pinned images and videos are being verified in pinned Messages adapter
@cool-appz
Related #58, #63, #62:
577fc26 
1- Added Document stub object to pinned messages
2- Added pinned Document xml views
3- Introduced isDocument() to PinnedMessagesAdapter
4- added some code cleanup in PinnedMessagesAdapter class
@cool-appz
Related #58: cleanup
9e08422
@ghost ghost assigned veken1199 Mar 9, 2018
@ghost ghost added the in progress label Mar 9, 2018
@veken1199 veken1199 added this to the Sprint 4 milestone Mar 9, 2018
g-harel
g-harel previously requested changes Mar 9, 2018
View reviewed changes
Copy link
Collaborator

@g-harel g-harel left a comment

Choose a reason for hiding this comment

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

Also should we test this behavior?

src/org/thoughtcrime/securesms/ConversationFragment.java Outdated
@@ -108,6 +108,7 @@
private View composeDivider;
private View scrollToBottomButton;
private TextView scrollDateHeader;
private int x =1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this being used anywhere? I can't find it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has slipped

src/org/thoughtcrime/securesms/PinnedMessageAdapter.java Outdated
holder.documentViewStub.get().setVisibility(view.VISIBLE);
}

if (isAudio(record)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this one also be else if?

src/org/thoughtcrime/securesms/PinnedMessageAdapter.java Outdated
holder.audioViewStub.get().setVisibility(view.VISIBLE);
}

else if (isVideo(record)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this should be on the same line as the closing bracket.

https://github.com/signalapp/Signal-Android/search?utf8=%E2%9C%93&q=else+if&type=Code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interesting
same goes for try catch

g-harel
g-harel approved these changes Mar 9, 2018
View reviewed changes
Copy link
Collaborator

@g-harel g-harel left a comment

Choose a reason for hiding this comment

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

👍

@veken1199 veken1199 merged commit f70c288 into master Mar 9, 2018
@ghost ghost removed the testing label Mar 9, 2018
@veken1199 veken1199 deleted the pin-message-video-audio-attachments branch March 9, 2018 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g-harel g-harel g-harel approved these changes

@Dan-Yoo Dan-Yoo Awaiting requested review from Dan-Yoo

@erinb90 erinb90 Awaiting requested review from erinb90

@Radu-Raicea Radu-Raicea Awaiting requested review from Radu-Raicea

@shelbaz shelbaz Awaiting requested review from shelbaz

Assignees

@veken1199 veken1199

Labels
Pin Messages PR
Projects
None yet
Milestone
Sprint 4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@veken1199 @g-harel @cool-appz