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

Add support for select multiple request. #1130

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

rohitjakhar
Copy link

📷 Screenshots

📄 Context

Issue link: Share Multiple Request at One Time

📝 Changes

Made change in TransactionAdapter to show color on Selected. Transaction, list of selected position, lambda for longClick on itemView.
Also Add query for get selected transaction data, delete transaction data.

🛠️ How to test

Long press at any transaction to select transaction, after that click other transaction to select them too.

Add support for share selected Transaction as text and har file type.
Add support for delete selected transaction only.
Fix Adapter parameters.
@rohitjakhar rohitjakhar requested a review from a team as a code owner November 22, 2023 07:55
@rohitjakhar
Copy link
Author

@cortinico what should i do next?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for sending this over. I've done a first pass. We need to do more testing as currently the logic is broken, see this video:

untitled

@@ -148,15 +168,16 @@ internal class MainActivity :
showDialog(
getClearDialogData(),
onPositiveClick = {
viewModel.clearTransactions()
if (selectedTransactions.isNotEmpty()) viewModel.clearSelectedTransactions(selectedTransactions) else viewModel.clearTransactions()
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this logic inside hte ViewModel? Let's just have a viewModel.clearTransactions(selectedTransactions) that behaves differently wether the selectedTransactions is empty or not

library/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
library/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@rohitjakhar
Copy link
Author

Thanks @cortinico for review, i will try to fix that ASAP.

@cortinico
Copy link
Member

@rohitjakhar Just to set expectations, I'll be able to take a look at this only next week.

…are_multiple_request

# Conflicts:
#	library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/MainActivity.kt
#	library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionAdapter.kt
@rohitjakhar
Copy link
Author

@cortinico I resolve conflicts, test all corner cases.

@cortinico
Copy link
Member

@rohitjakhar sorry I've been off for XMas vacation and haven't had time to look at this again. I can look into it this week so we can merge it. Could you fix the detekt/ktlint failures in the meanwhile?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks again for sending this over @rohitjakhar

Please reformat the code with ./gradlew ktlintFormat as formatting is odd in a couple of places.

Also there is a problem with rotation of the device: the highlights is lost, but the selection still remains. Also clicking delete is deleting the wrong transaction. See the video I'm attaching.

untitled

Comment on lines 27 to 30
) : ListAdapter<
HttpTransactionTuple,
TransactionAdapter.TransactionViewHolder>(
TransactionDiffCallback
Copy link
Member

Choose a reason for hiding this comment

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

The formatting here is odd and is causing ktlint to fail

}

@SuppressLint("SetTextI18n")
fun bind(transaction: HttpTransactionTuple) {
transactionId = transaction.id

if (selectedPos.find { it == adapterPosition } != null) {
itemView.setBackgroundColor(color400)
Copy link
Member

Choose a reason for hiding this comment

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

Let's not used this.
Let's create a new color resource as:

<color name="chucker_status_highlighted">#FF9800</color>

}
notifyItemChanged(adapterPosition)
true
} ?: kotlin.run { false }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} ?: kotlin.run { false }
} ?: false

Comment on lines 46 to 50
context.theme.resolveAttribute(
android.R.attr.selectableItemBackground,
outValue,
true,
)
Copy link
Member

Choose a reason for hiding this comment

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

formatting is odd also here

@cortinico
Copy link
Member

We're getting close to a mergeabble state :)

@rohitjakhar
Copy link
Author

@cortinico I fix above issues.

@rohitjakhar
Copy link
Author

@cortinico can you give me next step to merge it?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Let's make sure the CI is green 👍 You also need to run ./gradlew ktlintFormat before resubmitting

Comment on lines 175 to 181
getExportDialogData(
if (viewModel.isItemSelected.value == true) {
R.string.chucker_export_text_selected_http_confirmation
} else {
R.string.chucker_export_text_http_confirmation
},
),
Copy link
Member

Choose a reason for hiding this comment

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

Because of this, the function is now getting too long and letting detekt fail:


> Task :library:detekt
/home/runner/work/chucker/chucker/library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/MainActivity.kt:161:18: The function onOptionsItemSelected is too long (60). The maximum length is 60. [LongMethod]

You can see it in the CI signal.

Can you export this + lines 193/199 to separate functions

@rohitjakhar
Copy link
Author

@cortinico I just update code and run ktlintformat.

@rohitjakhar
Copy link
Author

@cortinico should i supress TooManyFunctions ?

@cortinico
Copy link
Member

@cortinico should i supress TooManyFunctions ?

Yes please do, it's reasonable to do so in MainActivity.kt

@rohitjakhar
Copy link
Author

@cortinico All are green

@cortinico
Copy link
Member

@rohitjakhar thanks for following up.

Sadly there are still a couple of bugs:

  1. The 4xx transactions are rendered impossible to read (see screnshot). We should probably change the text color to black when things are selected.

Screenshot_1718923015

  1. If you quickly select transactions as they're getting populated, you end up in a broken state where not all selected transactions are actually deleted. See video:
video-1718923534.mp4

Can I ask you to look into those 2 problems and then we can merge it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants