Warn before opening 5+ translation files
ClosedPublic

Authored by adrianchavesfernandez on Sep 15 2019, 8:47 AM.

Details

Summary

The context menu of a folder node in th Summary view of Lokalize
allows openning all the container files.

While this can be useful, accidentally selecting this option on the root
node of a large project (e.g. KDE translations) can be troublesome.

I’ve ripped off Dolphin’s approach for the same issue here: if more than
5 items are about to be opened, ask for user confirmation first.

I’ve used Dolphin’s limit (5), but I’m open to using a higher value for
Lokalize.

Test Plan

I tested the changes manually.

Diff Detail

Repository
R456 Lokalize
Branch
warn-before-opening-many-items (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16863
Build 16881: arc lint + arc unit
adrianchavesfernandez requested review of this revision.Sep 15 2019, 8:47 AM
adrianchavesfernandez created this revision.
ppeter requested changes to this revision.Sep 15 2019, 9:16 AM
ppeter added a subscriber: ppeter.
ppeter added inline comments.
src/project/projecttab.cpp
356

I think that is better to add the reason why the warning appears.

This revision now requires changes to proceed.Sep 15 2019, 9:16 AM
src/project/projecttab.cpp
356

Could you ellaborate? Maybe suggest an alternative text you would like?

shubham added inline comments.
src/project/projecttab.cpp
355

i >= 5, other KDE apps, for example Dolphin takes it into account

huftis added a subscriber: huftis.Sep 15 2019, 9:29 AM

You use a KMessageBox::warningYesNo() dialog box. Note that the KDE Human Interface Guidelines strongly recommend avoiding this (https://hig.kde.org/components/assistance/message.html):

Buttons should clearly indicate the available options using action verbs (“Delete”, “Rename”, “Close”, “Accept”, etc.) and allow the user to make an informed decision even if they have not read the message text. Never use “Yes” and “No” as button titles.

(The rest of the recommendations for modal dialogs are also worth reading and following.)

shubham added inline comments.Sep 15 2019, 9:31 AM
src/project/projecttab.cpp
355

Never mind, it is > 5,

adrianchavesfernandez marked 2 inline comments as done.Sep 15 2019, 9:37 AM

@huftis Any suggestion for the button names? Would “Open” and “Cancel” work?

Also, should I replace “item” with “file” in the dialog, since you cannot open folders with Lokalize?

src/project/projecttab.cpp
355

I actually copy-pasted the code from Dolphin :)

@huftis Any suggestion for the button names? Would “Open” and “Cancel” work?

Also, should I replace “item” with “file” in the dialog, since you cannot open folders with Lokalize?

Yes, call them “files”. Here’s a suggestion (which can probably be improved):

You are about to open 123 files. Opening a large number of files at the same time can make Lokalize unresponsive. Are you sure you want to open the files?

          Open All 123 Files | Cancel

shubham removed a subscriber: shubham.Sep 15 2019, 2:30 PM
adrianchavesfernandez marked an inline comment as done.Sep 21 2019, 9:52 AM

Should we use something other than 5? I copied it from Dolphin, but I believe for Lokalize it should be a higher value. 10? 20?

Applied changes based on the HIG-related suggestions

Also increased the required number of files from 5 to 50. In my PC,
Lokalize takes about 0.1 second per file, so with 50 I hope to warn
before an action that may take more than 10 seconds in a PC twice as
slow as mine, in line with the 10 second rule at
https://www.nngroup.com/articles/response-times-3-important-limits/

ppeter accepted this revision.Fri, Jan 3, 12:18 PM
This revision is now accepted and ready to land.Fri, Jan 3, 12:18 PM
This comment was removed by ppeter.

Oh, the "accept revision" is just like the feature "Approve" in GitHub and it is my expection. So I think I didn't do anything wrong :)

sdepiets accepted this revision.Sat, Jan 4, 10:54 AM