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.

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

Traders chat #3165

Merged
merged 35 commits into from Aug 30, 2019

Conversation

@chimp1984
Copy link
Contributor

commented Aug 29, 2019

Based on PR #2871 from @sqrrm.

I share the PR so that others can add optimisations for design and wording.

Open questions:

  • Do we need a flag in the settings to allow the user to not receive chat messages? Would require that the peer gets that information as well which is a bit of an effort. Maybe we can keep that for a later version if needed at all.
  • Do we need a popup or "news" badge to make sure users will find the new feature? I guess it is obvious enough, specially if you get a message.
  • Do we want a badge icon showing new messages if the selected screen is not pending trades?
  • Do we want to send mobile notifications?

I think all the above is not needed, at least not for a first release. But some UI beautification (@ripcurlx) and better wording (@m52go) would be nice...

Screen Shot 2019-08-30 at 01 13 20

sqrrm and others added 28 commits May 23, 2019
On the way to adding chat for traders this is a first step. Mainly just
moving functionality out of TraderDisputeView to Chat class. There are
still remaining dispute functionality that needs to be factored away.
The naming of DisputeCommunicationMessage has to stay but they otherwise
fit what would be more aptly named ChatCommunicationMessage or something
in that spririt.
Add extra button for generically added button in chat
Move session classes to core. Break out DisputeCommunicationMessage
handling from DisputeManager and put in ChatMananger prepare for other
uses of ChatManager.

Renaming of DisputeCommunicationMessage would be nice but it's
representing the protobuf messages so the name has to stay.
- Add communication messages to Trade protobuf message to be able to
save chat messages per trade
- Add Type enum and field to DisputeCommunicationMessage protobuf to
be able to dispatch Dispute and Trade chat messages properly
- Rename some function as isClient instead of isTrader to make it easier
to understand who is who when two traders are communicating with each
other
Very basic switch between chat and overview per trade, improvements
needed on the UI.
# Conflicts:
#	core/src/main/java/bisq/core/arbitration/DisputeManager.java
#	core/src/main/java/bisq/core/arbitration/messages/DisputeCommunicationMessage.java
#	core/src/main/java/bisq/core/trade/Trade.java
- Close already open chat if new one opens
# Conflicts:
#	desktop/src/main/java/bisq/desktop/main/disputes/trader/TraderDisputeView.java
- Fix logger
- Manage display state off trade chat msg (wip)
@battleofwizards

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

I think all the above is not needed, at least not for a first release.

I agree. None of the mentioned improvements sound critical for the first release. Better to release the thinnest vertically complete features possible.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@chimp1984 Looking great already! I'll give it a spin and provide feedback soon.

@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

I fixed the badge positioning and the tooltip font size. I'd also suggest following changes to the icon: Using a material design version of the chat icon and color it in our action color (green), which is more consistent throughout the app.
Bildschirmfoto 2019-08-30 um 12 37 25

…sign icon and add additional cleanup code
@ripcurlx

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

I just did more in-depth testing and everything works as expected. Also having arbitration case in parallel to the trader chat for a specific message doesn't cause any issues. Great work @sqrrm and @chimp1984 👍
@m52go Could you review the text content if you want to have some changes there?

If that is done I'm happy to merge this PR to master.

@ripcurlx ripcurlx requested a review from m52go Aug 30, 2019
Copy link
Member

left a comment

Text looks good to me overall...just a potential issue with links (see comment).

chimp1984 and others added 3 commits Aug 30, 2019
Co-Authored-By: Steve Jain <mfiver@gmail.com>
Co-Authored-By: Steve Jain <mfiver@gmail.com>
@chimp1984 chimp1984 marked this pull request as ready for review Aug 30, 2019
@chimp1984 chimp1984 requested review from ripcurlx and sqrrm as code owners Aug 30, 2019
- Store last position of the chat window so if it gets closed and opened
again it opens at the last position.

- Fix issues with the listener for new messages.
The handler was called multiple times before. Now its is called only
once. Tested with multiple trades and scrolling.

We use maps for each trade to avoid multiple listener registrations
when switching views. With current implementation we avoid that but we
do not remove listeners when a trade is removed (completed) but that
has no consequences as we will not receive any message anyway from a
closed trade. Supporting it more correctly would require more effort
and managing listener deactivation at screen switches (currently we
get the update called if we have selected another view.

This part can be improved if any dev feels motivated but its not
trivial...
@ripcurlx ripcurlx changed the title [WIP] Traders chat Traders chat Aug 30, 2019
@ripcurlx ripcurlx changed the title Traders chat [WIP] Traders chat Aug 30, 2019
@chimp1984 chimp1984 changed the title [WIP] Traders chat Traders chat Aug 30, 2019
Copy link
Member

left a comment

ACK

@ripcurlx ripcurlx merged commit b5f871b into bisq-network:master Aug 30, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chimp1984 chimp1984 deleted the chimp1984:trade-chat branch Aug 30, 2019
@@ -316,6 +316,19 @@ private void openChat(Trade trade) {
if (chatPopupStage != null)
chatPopupStage.close();

if (trade.getCommunicationMessages().isEmpty()) {

This comment has been minimized.

Copy link
@sqrrm

sqrrm Aug 30, 2019

Member

This will make receivers of messages from peers not see the code of conduct message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.