Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Dec 9, 2019

There was an issue around the time of Qt 4.6 when placeholder text was introduced, that caused a compile failure when it was specified in the form.

As a workaround the placeholder texts were moved to the code.

Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized) placeholder texts to the form files instead.

It's better to keep this kind of text content together. Translate/no-translate status is kept as it is.

Proof that they still work:
win1

win2

win3

win4

There was an issue around the time of Qt 4.6 when placeholder text was
introduced, that caused a compile failure when it was specified in the
form.

As a workaround the placeholder texts were moved to the code.

Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized)
placeholder texts to the form files instead.

It's better to keep this kind of text content together. Makes sure
translate/no-translate status is kept as it is.
@laanwj laanwj added the GUI label Dec 9, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 9, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15768 (gui: Add close window shortcut by IPGlider)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag
Copy link
Contributor

promag commented Dec 9, 2019

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 67f36e0, I have reviewed the code and it looks OK, I agree it can be merged.

Verified that other objects which use QLineEdit::setPlaceholderText() method have no corresponding *.ui forms.

@hebasto
Copy link
Member

hebasto commented Dec 9, 2019

Re-ACK a652dc5, tooltip and placeholderText are identical now.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK a652dc5 - checked that placeholder text still appears.

uri

@maflcko
Copy link
Member

maflcko commented Dec 9, 2019

Typed make translated and verified that the translations still work and exist.

ACK a652dc5 🚿

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK a652dc5521e2caf5734ffb797c7f2fc80685fef1 🚿
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgRiwwAw9cEXQKjQhNBOoI6jWuiup9tN+ZlstBfwFGUAHo2XFBoswA31k1poJOV
5AWVMk4dWRjlXqpwNGSUwl8elHGnvbEyaKJOHR5b5i2/Jed3LvmOvs+sWyxVVnYl
SAS+dD4iZG6CiWKA1WD6QEIFhBgVwTjIIlwrF1QDyu/EWrqI0T7JsbYW42VfBTfv
laPyGDkSw2DG7xLMHAfYNs1d+uY6x5oEd81XIdgvwxXenQKErBYfBmir6IpPjXIa
1ABSuHwqouEGN3MpuNYZX8zjbilKCkA3Ej7pBpcpcuCHQcXRBuQxKDkjpiuR9UPd
IVv38ZiPRKGLlpj2CxrhZJACgjf6I2fyPZ5ea0yzLI0iyp51hQigQ3m5SBrkVx2m
NiQ4uEmskPD8pej3IKyj1INwEX5shFAVDRw1TcToR6w32DjOWGJfi5JQ+ieTNLuI
SsmUGRZfCsREcSaTdtwvRM9BnoNyKsDQyK+tP0EfmyWevy2F39C76CwSvDQIEvMT
ziCMwOUJ
=3B7n
-----END PGP SIGNATURE-----

Timestamp of file with hash 1861b0f77d9a81d6407df8c3c7977e8b6bac8fa9dd0213dd982576c78ef08c68 -

maflcko pushed a commit that referenced this pull request Dec 9, 2019
a652dc5 qt: Normalize placeholder to avoid using "address book" in sendcoinsentry (Wladimir J. van der Laan)
67f36e0 gui: Move static placeholder texts to forms (Wladimir J. van der Laan)

Pull request description:

  There was an issue around the time of Qt 4.6 when placeholder text was introduced, that caused a compile failure when it was specified in the form.

  As a workaround the placeholder texts were moved to the code.

  Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized) placeholder texts to the form files instead.

  It's better to keep this kind of text content together. Translate/no-translate status is kept as it is.

  Proof that they still work:
  ![win1](https://user-images.githubusercontent.com/126646/70428014-0e80b300-1a76-11ea-9a6d-be78a0bf14ed.png)

  ![win2](https://user-images.githubusercontent.com/126646/70428019-10e30d00-1a76-11ea-8016-ffa0c4eafe34.png)

  ![win3](https://user-images.githubusercontent.com/126646/70428021-13456700-1a76-11ea-9449-9413487e39f6.png)

  ![win4](https://user-images.githubusercontent.com/126646/70428025-150f2a80-1a76-11ea-92ad-be5f3c171c43.png)

ACKs for top commit:
  hebasto:
    Re-ACK a652dc5, `tooltip` and `placeholderText` are identical now.
  MarcoFalke:
    ACK a652dc5 🚿
  fanquake:
    ACK a652dc5 - checked that placeholder text still appears.

Tree-SHA512: 7d3c1faeef2eb5d4b195d9d78f2a3f161296d869e5059b5e8d308167e3c6c668a3ebabec93dc592762ba15bfc86d51985e20c4e17f1065c8dce84fec036ff5ee
@maflcko maflcko merged commit a652dc5 into bitcoin:master Dec 9, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 9, 2019
a652dc5 qt: Normalize placeholder to avoid using "address book" in sendcoinsentry (Wladimir J. van der Laan)
67f36e0 gui: Move static placeholder texts to forms (Wladimir J. van der Laan)

Pull request description:

  There was an issue around the time of Qt 4.6 when placeholder text was introduced, that caused a compile failure when it was specified in the form.

  As a workaround the placeholder texts were moved to the code.

  Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized) placeholder texts to the form files instead.

  It's better to keep this kind of text content together. Translate/no-translate status is kept as it is.

  Proof that they still work:
  ![win1](https://user-images.githubusercontent.com/126646/70428014-0e80b300-1a76-11ea-9a6d-be78a0bf14ed.png)

  ![win2](https://user-images.githubusercontent.com/126646/70428019-10e30d00-1a76-11ea-8016-ffa0c4eafe34.png)

  ![win3](https://user-images.githubusercontent.com/126646/70428021-13456700-1a76-11ea-9449-9413487e39f6.png)

  ![win4](https://user-images.githubusercontent.com/126646/70428025-150f2a80-1a76-11ea-92ad-be5f3c171c43.png)

ACKs for top commit:
  hebasto:
    Re-ACK a652dc5, `tooltip` and `placeholderText` are identical now.
  MarcoFalke:
    ACK a652dc5 🚿
  fanquake:
    ACK a652dc5 - checked that placeholder text still appears.

Tree-SHA512: 7d3c1faeef2eb5d4b195d9d78f2a3f161296d869e5059b5e8d308167e3c6c668a3ebabec93dc592762ba15bfc86d51985e20c4e17f1065c8dce84fec036ff5ee
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Dec 11, 2019
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 6, 2020
Summary:
> There was an issue around the time of Qt 4.6 when placeholder text was
> introduced, that caused a compile failure when it was specified in the
> form. As a workaround the placeholder texts were moved to the code.
> Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized)
> placeholder texts to the form files instead.

The change to `openuridialog.cpp/.ui` is not included, because it is parametrized:
the placeholder text can be `bitcoincash:` or `bchtest:` or `bchreg:`

This is a backport of Core [[bitcoin/bitcoin#17702 | PR17702]]

Test Plan: `ninja && src/qt/bitcoin-qt`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8304
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
a652dc5 qt: Normalize placeholder to avoid using "address book" in sendcoinsentry (Wladimir J. van der Laan)
67f36e0 gui: Move static placeholder texts to forms (Wladimir J. van der Laan)

Pull request description:

  There was an issue around the time of Qt 4.6 when placeholder text was introduced, that caused a compile failure when it was specified in the form.

  As a workaround the placeholder texts were moved to the code.

  Qt 4 hasn't been relevant to us for ages. So move all (non-parametrized) placeholder texts to the form files instead.

  It's better to keep this kind of text content together. Translate/no-translate status is kept as it is.

  Proof that they still work:
  ![win1](https://user-images.githubusercontent.com/126646/70428014-0e80b300-1a76-11ea-9a6d-be78a0bf14ed.png)

  ![win2](https://user-images.githubusercontent.com/126646/70428019-10e30d00-1a76-11ea-8016-ffa0c4eafe34.png)

  ![win3](https://user-images.githubusercontent.com/126646/70428021-13456700-1a76-11ea-9449-9413487e39f6.png)

  ![win4](https://user-images.githubusercontent.com/126646/70428025-150f2a80-1a76-11ea-92ad-be5f3c171c43.png)

ACKs for top commit:
  hebasto:
    Re-ACK a652dc5, `tooltip` and `placeholderText` are identical now.
  MarcoFalke:
    ACK a652dc5 🚿
  fanquake:
    ACK a652dc5 - checked that placeholder text still appears.

Tree-SHA512: 7d3c1faeef2eb5d4b195d9d78f2a3f161296d869e5059b5e8d308167e3c6c668a3ebabec93dc592762ba15bfc86d51985e20c4e17f1065c8dce84fec036ff5ee
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants