Add memo field in UI #1493
Add memo field in UI #1493
Conversation
WalkthroughThis pull request introduces several changes to the user interface and functionality of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
levoncrypto
force-pushed
the
add_memo_field_UI
branch
from
cfdd569
to
b72db01
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/qt/sendcoinsentry.cpp (1)
91-94: Consider adding message field validation.While the message field functionality is implemented correctly, consider adding:
- Character length limits to prevent oversized messages
- Input validation to ensure message content meets any protocol requirements
- Visual feedback about remaining character count
This would improve user experience and prevent potential issues with transaction creation.
src/qt/forms/sendcoinsentry.ui (3)
234-239: Consider adding input constraints and placeholder text.The message field would benefit from additional properties:
- Maximum length constraint to prevent oversized messages
- Placeholder text to guide users on the expected input
<widget class="QLineEdit" name="messageTextLabel"> <property name="toolTip"> <string>Optional message for this transaction</string> </property> + <property name="maxLength"> + <number>140</number> + </property> + <property name="placeholderText"> + <string>Enter a message to include with the transaction</string> + </property> </widget>
Line range hint
729-738: Update tab order to include message field.The message field (messageTextLabel) should be included in the tab order sequence after the payment amount.
<tabstops> <tabstop>payTo</tabstop> <tabstop>addressBookButton</tabstop> <tabstop>pasteButton</tabstop> <tabstop>deleteButton</tabstop> <tabstop>addAsLabel</tabstop> <tabstop>payAmount</tabstop> + <tabstop>messageTextLabel</tabstop> <tabstop>payAmount_is</tabstop> <tabstop>deleteButton_is</tabstop> <tabstop>payAmount_s</tabstop> <tabstop>deleteButton_s</tabstop> </tabstops>
224-227: Consider harmonizing label text across payment forms.For consistency, consider using "Memo:" instead of "Message:" to match the label text used in both authenticated and unauthenticated payment request forms.
<widget class="QLabel" name="messageLabel"> <property name="text"> - <string>Message:</string> + <string>Memo:</string> </property> <property name="alignment"> <set>Qt::AlignRight|Qt::AlignTrailing|Qt::AlignVCenter</set> </property> </widget>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/qt/forms/sendcoinsentry.ui(2 hunks)src/qt/sendcoinsdialog.cpp(2 hunks)src/qt/sendcoinsentry.cpp(2 hunks)src/qt/transactiondesc.cpp(1 hunks)src/qt/walletmodel.cpp(2 hunks)
🔇 Additional comments (7)
src/qt/sendcoinsentry.cpp (2)
58-60: LGTM: Proper initialization of message field visibility.
The message fields are correctly initialized as hidden by default in the constructor.
91-94: Verify consistency with other message field implementations.
Let's check if there are other similar message field implementations in the codebase to ensure consistent behavior.
✅ Verification successful
Let me gather more specific information about the message field implementations.
Based on the gathered information, I can now provide the final response:
Message field implementation is consistent with codebase patterns
The implementation correctly follows the established patterns in the codebase:
- The message field visibility is properly tied to Spark addresses through
validateSparkAddress - The validation logic is consistently used across the codebase for Spark-related functionality
- The UI implementation in
sendcoinsentry.uiis the only instance of this specific message field, which is appropriate as it's the dedicated send coins form
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other message field implementations in Qt UI files
# and related C++ files to ensure consistent validation and visibility behavior
# Check for other message field implementations in UI files
echo "Checking UI files for message field implementations..."
fd -e ui | xargs grep -l "messageTextLabel\|messageLabel"
# Check for message validation patterns in C++ files
echo "Checking C++ files for message validation patterns..."
fd -e cpp -e h | xargs grep -l "validateSparkAddress\|messageTextLabel"
Length of output: 760
Script:
#!/bin/bash
# Check the actual implementation of message fields and validation in related files
echo "Checking message field implementations in sendcoinsentry.ui..."
cat src/qt/forms/sendcoinsentry.ui
echo -e "\nChecking validateSparkAddress implementations..."
rg "validateSparkAddress" -A 5
echo -e "\nChecking message visibility patterns..."
rg "setVisible.*message" -A 2
Length of output: 48440
src/qt/transactiondesc.cpp (1)
314-321: Verify transaction message handling across the codebase.
Let's ensure consistent message handling across the UI components.
✅ Verification successful
Let me gather more specific information about the message handling implementation.
Transaction message handling is consistent across the codebase
The code changes are safe as the implementation maintains consistency with existing message handling patterns:
- The Spark memo handling in
transactiondesc.cppfollows the same HTML formatting pattern used for regular transaction messages (GUIUtil::HtmlEscapewith proper translation) - The memo field is properly integrated in the wallet model (
walletmodel.cpp) for transaction creation - The Spark wallet implementation (
sparkwallet.cpp) handles memos consistently throughout its operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other transaction message handling implementations
# that might need similar updates.
echo "Checking for transaction message handling in UI components..."
rg -l "mapValue\[\"message\"\]" src/qt/
echo "Checking for memo field usage..."
rg -l "\.memo" src/
echo "Checking for other spark message implementations..."
ast-grep --pattern 'strHTML += $_ + tr("Message") + $_'
Length of output: 1465
Script:
#!/bin/bash
# Check the specific implementation in transactiondesc.cpp
echo "Checking transaction message implementations in transactiondesc.cpp..."
rg -A 5 "Message" src/qt/transactiondesc.cpp
# Check wallet model implementation
echo "Checking walletmodel.cpp memo handling..."
rg -A 5 "memo" src/qt/walletmodel.cpp
# Check sparkwallet implementation
echo "Checking sparkwallet.cpp memo handling..."
rg -A 5 "memo" src/spark/sparkwallet.cpp
# Look for any other UI message formatting
echo "Checking for other UI message formatting patterns..."
rg "HtmlEscape.*message" src/qt/
Length of output: 4031
src/qt/sendcoinsdialog.cpp (1)
268-269: Verify SendCoinsRecipient struct changes.
The message capture looks good, but we should verify that the SendCoinsRecipient struct has been properly updated to include the message field.
✅ Verification successful
Based on the search results, I can now generate the final response:
The SendCoinsRecipient struct has proper message field support
The message field is properly defined in the SendCoinsRecipient struct and is consistently used throughout the codebase:
- Defined in
src/qt/walletmodel.hwith constructor support - Used in transaction creation in
src/qt/walletmodel.cpp - Properly handled in URI parsing/formatting in
src/qt/guiutil.cpp - Correctly integrated with the UI components in
src/qt/sendcoinsentry.cpp
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SendCoinsRecipient struct has message field
# Search for SendCoinsRecipient struct definition
rg -A 10 "struct SendCoinsRecipient"
Length of output: 38
Script:
#!/bin/bash
# Let's try different approaches to find the struct definition
# 1. Search in header files
fd -e h -e hpp | xargs rg -l "SendCoinsRecipient"
# 2. Search for any mention of SendCoinsRecipient
rg "SendCoinsRecipient" -A 5 -B 5
# 3. Search specifically in Qt-related directories
fd -e h -e hpp -p ".*qt.*" | xargs rg "SendCoinsRecipient"
Length of output: 45650
src/qt/walletmodel.cpp (3)
1387-1387: LGTM: Memo field correctly integrated into Spark minting.
The memo field is properly populated from the recipient's message and converted to the appropriate string type.
1484-1484: LGTM: Memo field correctly integrated into Spark spending.
The memo field is properly populated from the recipient's message and converted to the appropriate string type.
1387-1387: Overall implementation looks good.
The memo field has been consistently integrated into both minting and spending transactions, maintaining the existing error handling and validation patterns. The implementation is clean and focused on the PR objective.
Also applies to: 1484-1484
levoncrypto
force-pushed
the
add_memo_field_UI
branch
from
dd2fdae
to
dc58b11
Compare
levoncrypto
force-pushed
the
add_memo_field_UI
branch
from
03c4906
to
3e7f4ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
src/qt/sendcoinsentry.h (2)
66-66: Add documentation for the new slot method.The new slot method looks good and follows Qt conventions. However, consider adding a brief documentation comment explaining its purpose and the 256-character limit mentioned in the implementation.
+ /** Handles memo text changes and enforces the 256-character limit */ void on_MemoTextChanged(const QString &text);
66-66: Consider consistent naming between UI and code.The slot name uses "MemoText" while the UI element is named "messageTextLabel". Consider aligning these names for better code maintainability.
Choose one of these alternatives:
- void on_MemoTextChanged(const QString &text); + void on_messageTextChanged(const QString &text);or update the UI element name to match:
- messageTextLabel → memoTextEditsrc/qt/sendcoinsentry.cpp (3)
32-32: Consider grouping related UI element initializations.The message-related UI element initializations are split between the icon setup and visibility settings. Consider grouping them together for better code organization.
QIcon icon_; icon_.addFile(QString::fromUtf8(":/icons/ic_warning"), QSize(), QIcon::Normal, QIcon::On); ui->iconWarning->setPixmap(icon_.pixmap(18, 18)); ui->iconMessageWarning->setPixmap(icon_.pixmap(18, 18)); + +// Initialize message-related UI elements +ui->messageLabel->setVisible(false); +ui->messageTextLabel->setVisible(false); +ui->iconMessageWarning->setVisible(false); ui->addressBookButton->setIcon(platformStyle->SingleColorIcon(":/icons/address-book")); - -ui->messageLabel->setVisible(false); -ui->messageTextLabel->setVisible(false); -ui->iconMessageWarning->setVisible(false);Also applies to: 61-63
71-87: Improve code quality and maintainability of the memo text handler.Several improvements can be made to enhance the code:
- Define the character limit as a constant
- Use early return pattern
- Extract UI state management into a helper method
+// At class level +static const int MAX_MEMO_LENGTH = 256; + void SendCoinsEntry::on_MemoTextChanged(const QString &text) { - int maxLength = 256; - bool isOverLimit = text.length() > maxLength; + bool isOverLimit = text.length() > MAX_MEMO_LENGTH; + setMemoWarningState(isOverLimit); +} - if (isOverLimit) { - ui->messageWarning->setText("Message exceeds character 256 character limit"); - ui->messageWarning->setVisible(true); - ui->messageTextLabel->setStyleSheet("border: 1px solid red;"); - ui->iconMessageWarning->setVisible(true); - } else { - ui->messageWarning->clear(); - ui->messageWarning->setVisible(false); - ui->messageTextLabel->setStyleSheet(""); - ui->iconMessageWarning->setVisible(false); - } +private: +void SendCoinsEntry::setMemoWarningState(bool isOverLimit) +{ + ui->messageWarning->setText(isOverLimit ? + QString("Message exceeds character %1 character limit").arg(MAX_MEMO_LENGTH) : + QString()); + ui->messageWarning->setVisible(isOverLimit); + ui->messageTextLabel->setStyleSheet(isOverLimit ? "border: 1px solid red;" : ""); + ui->iconMessageWarning->setVisible(isOverLimit); }🧰 Tools
🪛 cppcheck
[performance] 80-80: Function parameter 'label' should be passed by const reference.
(passedByValue)
[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.
(passedByValue)
[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
113-118: Improve readability of address validation code.The implementation correctly handles the null check for the model. Consider extracting the visibility logic into a well-named constant for better readability.
bool isSparkAddress = false; if (model) { isSparkAddress = model->validateSparkAddress(address); } -ui->messageLabel->setVisible(isSparkAddress); -ui->messageTextLabel->setVisible(isSparkAddress); +const bool shouldShowMessageFields = isSparkAddress; +ui->messageLabel->setVisible(shouldShowMessageFields); +ui->messageTextLabel->setVisible(shouldShowMessageFields);src/qt/forms/sendcoinsentry.ui (2)
Line range hint
224-240: Consider adding input constraints and placeholder text.The message input field would benefit from additional properties:
- Add a maximum length constraint to prevent oversized messages
- Include placeholder text to guide users on the expected input
<widget class="QLineEdit" name="messageTextLabel"> <property name="toolTip"> <string>Optional message for this transaction</string> </property> + <property name="maxLength"> + <number>140</number> + </property> + <property name="placeholderText"> + <string>Enter an optional message to include with the transaction</string> + </property> </widget>
Line range hint
224-281: Add accessibility properties for screen readers.Consider adding accessibility properties to improve usability for users with screen readers:
<widget class="QLineEdit" name="messageTextLabel"> <property name="toolTip"> <string>Optional message for this transaction</string> </property> + <property name="accessibleName"> + <string>Transaction message input</string> + </property> + <property name="accessibleDescription"> + <string>Enter an optional message to be included with this transaction</string> + </property> </widget> <widget class="QLabel" name="iconMessageWarning"> + <property name="accessibleName"> + <string>Message warning icon</string> + </property> </widget> <widget class="QLabel" name="messageWarning"> + <property name="accessibleName"> + <string>Message warning text</string> + </property> </widget>src/qt/sendcoinsdialog.cpp (1)
536-547: Improve message section display in confirmation dialog.While the implementation properly sanitizes messages using
HtmlEscape, consider these improvements:
- Only show the "Messages" section header if there are actual messages to display
- Add length limits to prevent dialog layout issues with very long messages
Apply this diff to improve the implementation:
bool firstMessage = true; +bool hasMessages = false; + +// First check if we have any messages +for (const auto& rec : recipients) { + if (!rec.message.isEmpty()) { + hasMessages = true; + break; + } +} + +if (hasMessages) { + questionString.append("<hr><b>" + tr("Messages") + ":</b><br>"); +} for (const auto& rec : recipients) { if (!rec.message.isEmpty()) { - if (firstMessage) { - questionString.append("<hr><b>" + tr("Messages") + ":</b><br>"); - firstMessage = false; - } QString sanitizedMsg = GUIUtil::HtmlEscape(rec.message, true); + // Limit message length for display + if (sanitizedMsg.length() > 500) { + sanitizedMsg = sanitizedMsg.left(500) + "..."; + } questionString.append("• " + sanitizedMsg + "<br>"); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/qt/forms/sendcoinsentry.ui(2 hunks)src/qt/sendcoinsdialog.cpp(2 hunks)src/qt/sendcoinsentry.cpp(3 hunks)src/qt/sendcoinsentry.h(1 hunks)src/qt/transactiondesc.cpp(1 hunks)src/qt/walletmodel.cpp(2 hunks)src/spark/primitives.h(3 hunks)src/spark/sparkwallet.cpp(3 hunks)src/wallet/wallet.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/qt/transactiondesc.cpp
- src/qt/walletmodel.cpp
🧰 Additional context used
🪛 cppcheck
src/qt/sendcoinsentry.cpp
[performance] 80-80: Function parameter 'label' should be passed by const reference.
(passedByValue)
[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.
(passedByValue)
[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
🔇 Additional comments (9)
src/spark/primitives.h (3)
110-110: Verify serialization backward compatibility.
The addition of the memo field to serialization could affect backward compatibility with existing stored data or network messages. The placement at the end of the serialization sequence is good practice, but we should ensure proper version handling.
Let's verify if there's version handling in place:
#!/bin/bash
# Search for version handling in serialization
rg -B 5 -A 5 "SerializationOp.*version|SERIALIZE.*version" Line range hint 90-110: Verify UI integration with primitive changes.
The changes to CSparkOutputTx look good, but since this PR's objective is "Add memo field in UI", we should ensure proper integration between the UI layer and these primitive changes.
Let's verify the UI-primitive integration:
#!/bin/bash
# Search for UI components using CSparkOutputTx
rg -l "CSparkOutputTx.*memo|memo.*CSparkOutputTx" --type cpp --type h
# Search for related UI files
fd -e ui | xargs rg -l "memo"90-90: Consider adding memo length constraints.
While the memo field is properly initialized, it might be beneficial to define maximum length constraints to prevent potential abuse or excessive memory usage. This is particularly important since this structure is serialized.
Let's verify if memo length constraints are defined elsewhere:
Also applies to: 101-101
src/qt/sendcoinsentry.cpp (1)
Line range hint 1-324: Implementation looks good overall!
The memo field functionality is well-implemented with appropriate validation and UI feedback. The code handles null checks correctly and maintains good separation of concerns. The suggested improvements are minor and focus on code organization and readability.
🧰 Tools
🪛 cppcheck
[performance] 80-80: Function parameter 'label' should be passed by const reference.
(passedByValue)
[performance] 90-90: Function parameter 'data' should be passed by const reference.
(passedByValue)
[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.
(passedByValue)
[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
src/qt/forms/sendcoinsentry.ui (1)
241-281: LGTM! Warning layout follows existing patterns.
The implementation of the message warning layout:
- Maintains consistent styling with other warnings
- Uses proper margin alignment
- Follows the established pattern for warning displays in the UI
src/qt/sendcoinsdialog.cpp (1)
268-268: Add message validation before assignment.
The message is assigned directly from the entry without any validation or sanitization, which could lead to security issues.
The previous review comment about adding message validation and length limits is still valid and should be addressed.
src/spark/sparkwallet.cpp (3)
745-746: LGTM: Memo field properly added to CRecipient
The memo field is correctly extracted from the outputs and initialized in the CRecipient structure.
1097-1097: LGTM: Memo field properly persisted in wallet database
The memo field is correctly stored in the CSparkOutputTx structure when writing to the wallet database.
1535-1535: LGTM: Memo field properly handled in private outputs
The memo field is correctly stored in the CSparkOutputTx structure for private outputs when writing to the wallet database.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/qt/sendcoinsentry.cpp(4 hunks)src/spark/primitives.h(3 hunks)src/spark/sparkwallet.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spark/primitives.h
🧰 Additional context used
📓 Learnings (1)
src/spark/sparkwallet.cpp (1)
Learnt from: levonpetrosyan93
PR: firoorg/firo#1493
File: src/spark/sparkwallet.cpp:745-746
Timestamp: 2024-11-10T23:59:00.992Z
Learning: When validating memo fields in Spark transactions, use `get_memo_bytes()` from the Spark parameters to get the maximum allowed memo size.
🪛 cppcheck
src/qt/sendcoinsentry.cpp
[performance] 80-80: Function parameter 'label' should be passed by const reference.
(passedByValue)
[performance] 90-90: Function parameter 'data' should be passed by const reference.
(passedByValue)
[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.
(passedByValue)
[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
src/spark/sparkwallet.cpp
[performance] 750-750: Searching before insertion is not necessary. Instead of 'mapKeyMetadata[keyid]=CKeyMetadata(keypool.nTime)' consider using 'mapKeyMetadata.try_emplace(keyid, CKeyMetadata(keypool.nTime));'.
(stlFindInsert)
🔇 Additional comments (3)
src/qt/sendcoinsentry.cpp (2)
14-14: LGTM: Initialization changes are well-structured
The initialization of message-related UI elements and signal connections follows Qt best practices.
Also applies to: 33-33, 60-64
114-120: LGTM: Address validation and UI visibility logic
The implementation correctly handles model validation and UI element visibility based on Spark address validation. The null check for the model is properly implemented as suggested in previous reviews.
src/spark/sparkwallet.cpp (1)
1101-1101: LGTM: Consistent memo storage implementation
The memo field is correctly stored in the output transactions for both mint and spend operations.
Also applies to: 1539-1539
levoncrypto
force-pushed
the
add_memo_field_UI
branch
from
6546d17
to
fd6a779
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/qt/sendcoinsentry.cpp (1)
72-72: Add function documentation.Add a documentation block explaining the purpose and behavior of this function.
+/** + * Handles memo text changes, validates length against memo size limit, + * sanitizes input, and updates UI feedback accordingly. + * @param text The new memo text + */ void SendCoinsEntry::on_MemoTextChanged(const QString &text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/qt/sendcoinsentry.cpp(4 hunks)src/spark/primitives.h(3 hunks)src/spark/sparkwallet.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spark/primitives.h
🧰 Additional context used
📓 Learnings (1)
src/spark/sparkwallet.cpp (1)
Learnt from: levonpetrosyan93
PR: firoorg/firo#1493
File: src/spark/sparkwallet.cpp:745-746
Timestamp: 2024-11-10T23:59:00.992Z
Learning: When validating memo fields in Spark transactions, use `get_memo_bytes()` from the Spark parameters to get the maximum allowed memo size.
🪛 cppcheck
src/qt/sendcoinsentry.cpp
[performance] 80-80: Function parameter 'label' should be passed by const reference.
(passedByValue)
[performance] 90-90: Function parameter 'data' should be passed by const reference.
(passedByValue)
[performance] 74-74: Function parameter 'hash_bind_inner' should be passed by const reference.
(passedByValue)
[performance] 83-83: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
src/spark/sparkwallet.cpp
[performance] 750-750: Searching before insertion is not necessary. Instead of 'mapKeyMetadata[keyid]=CKeyMetadata(keypool.nTime)' consider using 'mapKeyMetadata.try_emplace(keyid, CKeyMetadata(keypool.nTime));'.
(stlFindInsert)
🔇 Additional comments (4)
src/qt/sendcoinsentry.cpp (2)
14-14: LGTM: Constructor changes properly initialize memo-related UI elements.
The initialization of memo-related UI elements is well-structured:
- Message elements are hidden by default
- Warning icon is properly initialized
- Signal connection for memo text changes is correctly set up
Also applies to: 33-33, 60-64
120-126: LGTM: Address validation properly handles memo field visibility.
The implementation correctly:
- Checks for null model before validation
- Shows/hides memo fields only for Spark addresses
src/spark/sparkwallet.cpp (2)
1101-1101: LGTM!
The memo field is correctly added to the CSparkOutputTx structure.
1539-1539: LGTM!
The memo field is correctly assigned from the private recipient to the output transaction.
No description provided.