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

Make URLs in popup notices clickable #4637

Merged
merged 2 commits into from Oct 16, 2020
Merged

Make URLs in popup notices clickable #4637

merged 2 commits into from Oct 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 13, 2020 •
edited by ghost

Fixes #4623

URLs in Javafx label controls are not clickable, so separate them out to one or more clickable Hyperlinks displayed at the footer of the popup.

The approach taken for this is to embed the tag [HYPERLINK:https://foo.bar] in the message around a link that you want to be handled this way. The links can be anywhere in the popup message. They will be extracted, referenced and listed at the bottom of the popup. Using the tag has the advantage of being optional, and does not require new code to be written for each individual popup.

These popup messages have been modified to use the [HYPERLINK:] tag:

  • offerbook.warning.newVersionAnnouncement
  • offerbook.warning.requireUpdateToNewVersion
  • portfolio.pending.step2_buyer.confirmStart.proof.noneProvided
  • portfolio.pending.openAgainDispute.msg
  • portfolio.pending.mediationResult.popup.info
  • portfolio.pending.mediationResult.popup.selfAccepted.lockTimeOver
  • setting.info.msg
  • account.altcoin.popup.xmr.msg (2 hyperlinks)
  • account.seed.backup.warning
  • displayUpdateDownloadWindow.download.failed
  • displayUpdateDownloadWindow.installer.failed
  • displayUpdateDownloadWindow.verify.failed
  • popup.info.qubesOSSetupInfo
  • popup.accountSigning.generalInformation
  • payment.clearXchange.info
  • payment.limits.info.withSigning
  • payment.f2f.info
  • portfolio.pending.invalidDelayedPayoutTx (2 hyperlinks)
  • portfolio.pending.failedTrade.missingDepositTx
  • portfolio.pending.failedTrade.buyer.existingDepositTxButMissingDelayedPayoutTx
  • portfolio.pending.failedTrade.seller.existingDepositTxButMissingDelayedPayoutTx
  • portfolio.pending.failedTrade.errorMsgSet
  • portfolio.pending.failedTrade.missingContract
  • settings.net.warn.usePublicNodes
  • popup.warning.incompatibleDB

Examples:

bisq-account-altcoin-popup-xmr

bisq-setting-info-msg

bisq-payment-clearXchange-info

cd2357
cd2357 suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@cd2357 cd2357 left a comment

Choose a reason for hiding this comment

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

Found an extra dot in one of the links, otherwise ACK from me.

core/src/main/resources/i18n/displayStrings.properties Outdated Show resolved Hide resolved
chimp1984
chimp1984 previously approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

Great thanks for the prompt implementation! Cool solution!

@ghost ghost marked this pull request as draft October 14, 2020 04:10
@ghost
Copy link
Author

ghost commented Oct 14, 2020 •
edited by ghost

Testing status:

[TESTED]- offerbook.warning.newVersionAnnouncement
[TESTED]- offerbook.warning.requireUpdateToNewVersion
[TESTED]- portfolio.pending.step2_buyer.confirmStart.proof.noneProvided
[TESTED]- portfolio.pending.openAgainDispute.msg
[TESTED]- portfolio.pending.mediationResult.popup.info
[TESTED]- portfolio.pending.mediationResult.popup.selfAccepted.lockTimeOver
[TESTED]- setting.info.msg
[TESTED]- account.altcoin.popup.xmr.msg (2 hyperlinks)
[TESTED]- account.seed.backup.warning
[TESTED]- displayUpdateDownloadWindow.download.failed
[TESTED]- displayUpdateDownloadWindow.installer.failed
[TESTED]- displayUpdateDownloadWindow.verify.failed
[TESTED]- popup.info.qubesOSSetupInfo
[TESTED]- popup.accountSigning.generalInformation
[TESTED]- payment.clearXchange.info
[TESTED]- payment.limits.info.withSigning
[TESTED]- payment.f2f.info
[TESTED]- portfolio.pending.invalidDelayedPayoutTx (2 hyperlinks)
[TESTED]- portfolio.pending.failedTrade.missingDepositTx
[TESTED]- portfolio.pending.failedTrade.buyer.existingDepositTxButMissingDelayedPayoutTx
[TESTED]- portfolio.pending.failedTrade.seller.existingDepositTxButMissingDelayedPayoutTx
[TESTED]- portfolio.pending.failedTrade.errorMsgSet
[TESTED]- portfolio.pending.failedTrade.missingContract
[TESTED]- settings.net.warn.usePublicNodes
[TESTED]- popup.warning.incompatibleDB

Screenshots of all dialogs tested: https://imgur.com/a/pwP06gB

custom error box, not a popup
[REVERTED] - torNetworkSettingWindow.bridges.info
[REVERTED] - popup.reportError

[NOT USED ANYWHERE!] - popup.warning.trade.depositTxNull

@ghost ghost marked this pull request as ready for review October 14, 2020 18:30
Make URLs in popup notices clickable
b17c5c9 
Fixes #4623

URLs in Javafx label controls are not clickable, so separate them out to
one or more clickable Hyperlinks displayed at the footer of the popup.

The approach taken for this is to embed the tag `[HYPERLINK:]` in the
message around a link that you want to be handled this way.  The links
can be anywhere within the popup message, in the same order that they
will be displayed.  Using the tag has the advantage of being optional,
and does not require new code to be written for each individual popup.

Refactor to be more flexible; allowing inline hyperlinks

Codacy nits.
@ghost
Copy link
Author

ghost commented Oct 14, 2020

Status update: testing revealed some of the more complex popups (e.g. missingDepositTx) were not compatible with the first implementation of this feature. So I re-implemented the parsing to extract hyperlinks from anywhere within the textual message using regex Pattern/Matcher and String.replaceFirst.

The code has been fully retested as documented here and is ready for review. (Apologies to those who already reviewed the prior version).

N.B. I came across a message which appears unused in Bisq: popup.warning.trade.depositTxNull. Could someone confirm that? Maybe it should be removed from displayStrings.properties now.

chimp1984
chimp1984 previously approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

@chimp1984
Copy link
Contributor

Re "popup.warning.trade.depositTxNull": Yes that and the other related ones are not used anymore and should be deleted in the strings file.

chimp1984
chimp1984 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

@cd2357
Copy link
Contributor

cd2357 commented Oct 15, 2020

utACK

ripcurlx
ripcurlx approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK - code looks fine 👍

@ripcurlx ripcurlx merged commit 4a20cc8 into bisq-network:master Oct 16, 2020
chimp1984 pushed a commit to chimp1984/bisq that referenced this pull request Oct 19, 2020
@chimp1984
Remove unused strings, ref:
9f228af 
bisq-network#4637 (comment)
@ghost ghost mentioned this pull request Oct 23, 2020
Closed
@ghost ghost mentioned this pull request Nov 26, 2020
Closed
@ghost ghost deleted the clickable_urls branch December 3, 2020 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

If there are links in popups add a clickable foot note
3 participants
@chimp1984 @cd2357 @ripcurlx