Page MenuHomePhorge
Differential D104

Fix crash when upload failed
ClosedPublic
Actions

Authored by nannanko on Jul 6 2024, 5:10 PM.

Details

Reviewers
tusooa
Group Reviewers
O1: the Kazv Project
Maniphest Tasks
T106: App will crash if uploading file failed
Commits
rK4bb140bd8261: Fix crash when upload failed
Summary

When response isn't an valid json, nlohmann::json::parse() will cause a crash. This fixed it.

Type: fix

Test Plan

Verify that kazv doesn't crash when upload failed.

Diff Detail

Repository
rK kazv
Branch
servant
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 170
Build 329: GitLab CI for kazv
Build 328: arc lint + arc unit

Event Timeline

nannanko created this revision.Jul 6 2024, 5:10 PM
Owners added a reviewer: O1: the Kazv Project.Jul 6 2024, 5:10 PM
nannanko added a task: T106: App will crash if uploading file failed.Jul 6 2024, 5:11 PM
nannanko added a comment.Jul 6 2024, 5:14 PM

There is currently a lack of unit tests, and I'm not sure how to write them at the moment. I will add all related tests for uploading and downloading soon.

Harbormaster completed remote builds in B148: Diff 258.Jul 6 2024, 5:26 PM
nannanko requested review of this revision.Jul 6 2024, 5:26 PM
tusooa added inline comments.Jul 6 2024, 5:41 PM
src/kazv-io-job.cpp
299–300

Why not use try-catch?

Also Better to log it.

299–300

Don't assume the json body is in valid format. There are plenty of things that can go wrong.

tusooa requested changes to this revision.Jul 6 2024, 5:41 PM
This revision now requires changes to proceed.Jul 6 2024, 5:41 PM
nannanko updated this revision to Diff 280.Jul 12 2024, 6:50 AM
  • Use try-catch for exception
nannanko marked 2 inline comments as done.Jul 12 2024, 6:52 AM
Harbormaster completed remote builds in B166: Diff 280.Jul 12 2024, 7:07 AM
tusooa requested changes to this revision.Jul 12 2024, 7:08 AM
tusooa added inline comments.
src/kazv-io-job.cpp
300–315

Note, your original code doesn't create a default-constructed json. This creates an empty json array instead.

305

should be qCWarning(kazvLog)

309
310
313

should be qCWarning(kazvLog)

src/l10n/en/100-ui.ftl
212

missing translations for cmn-Hans

This revision now requires changes to proceed.Jul 12 2024, 7:08 AM
nannanko updated this revision to Diff 289.Jul 13 2024, 5:52 AM
  • Add cmn-Hans translations
  • Use qCWarning instead of qDebug
  • Check json format, fix a template call
nannanko marked 6 inline comments as done.Jul 13 2024, 5:53 AM
Harbormaster completed remote builds in B170: Diff 289.Jul 13 2024, 6:08 AM
tusooa accepted this revision.Jul 13 2024, 9:08 AM
This revision is now accepted and ready to land.Jul 13 2024, 9:08 AM
Closed by commit rK4bb140bd8261: Fix crash when upload failed. · Explain WhyJul 13 2024, 9:10 AM
This revision was automatically updated to reflect the committed changes.
tusooa added a commit: rK4bb140bd8261: Fix crash when upload failed.

Revision Contents
Changeset List

Diff 289

View Options

src/contents/ui/KazvIOMenu.qml

Loading...
View Options

src/kazv-io-job.hpp

Loading...
View Options

src/kazv-io-job.cpp

Loading...
View Options

src/l10n/cmn-Hans/100-ui.ftl

Loading...
View Options

src/l10n/en/100-ui.ftl

Loading...