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

Allow multiple image selections when adding favicons #2011

Merged
merged 3 commits into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@droidmonkey
Copy link
Member

commented May 31, 2018

Description

Quick tweak to let users select multiple icons when adding favicons from the filesystem. Useful for testing too!

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@droidmonkey droidmonkey added this to the v2.3.4 milestone May 31, 2018

@droidmonkey droidmonkey requested a review from phoerious May 31, 2018

if (!icon.isNull()) {
addCustomIcon(QImage(filename));
} else {
emit messageEditEntry(tr("Can't read icon"), MessageWidget::Error);

This comment has been minimized.

Copy link
@TheZ3ro

TheZ3ro Jun 3, 2018

Contributor

An improvement: report in the message the filename that can't be loaded.

Also if I load 100 corrupted icons this message will be "spammed" 100 times.
IMHO this needs to be reported at the end of the for cycle with a list of the error filenames.
If the list is too long display only the first N filename

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Jun 3, 2018

Author Member

This was way more complicated than it needed to be, but after some replumbing it works great!

@droidmonkey droidmonkey force-pushed the fix/favicon-multiselect branch from 1d09b90 to 75aef53 Jun 3, 2018

@TheZ3ro

TheZ3ro approved these changes Jun 4, 2018

Copy link
Contributor

left a comment

Great!

if (!filename.isEmpty()) {
auto icon = QImage(filename);
if (!icon.isNull()) {
if (!addCustomIcon(QImage(filename))) {

This comment has been minimized.

Copy link
@phoerious

phoerious Jun 8, 2018

Member

Combine conditions.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Jun 8, 2018

Author Member

They have to be separate because I want to fall through (ie, not an error).

This comment has been minimized.

Copy link
@phoerious

phoerious Jun 9, 2018

Member

That would be else if (icon.isNull()).

}

if (numexisting > 0) {
msg += ", " + tr("%1 icons already existed").arg(numexisting);

This comment has been minimized.

Copy link
@phoerious

phoerious Jun 8, 2018

Member

This doesn't work. You cannot assume other languages to append clauses in this order or use commas to separate them. This needs to be two separate messages and it needs proper plural handling as well.

QString msg;

if (numloaded > 0) {
msg = tr("Successfully loaded %1 of %2 icons").arg(numloaded).arg(filenames.size());

This comment has been minimized.

Copy link
@phoerious

phoerious Jun 8, 2018

Member

Missing plural handling.

if (!errornames.empty()) {
// Show the first 8 icons that failed to load
errornames = errornames.mid(0, 8);
emit messageEditEntry(msg + "\n" + tr("The following icons failed:") + "\n" + errornames.join("\n"),

This comment has been minimized.

Copy link
@phoerious

phoerious Jun 8, 2018

Member

See above.

droidmonkey added some commits May 31, 2018

Add more comprehensive messages when adding custom icons
* Error messages now display for 15 seconds and are closable
* Add button is always enabled

@droidmonkey droidmonkey force-pushed the fix/favicon-multiselect branch from 75aef53 to 76102ee Jun 9, 2018

@droidmonkey droidmonkey changed the base branch from release/2.3.4 to develop Jun 9, 2018

@droidmonkey droidmonkey modified the milestones: v2.3.4, v2.4.0 Jun 9, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2018

Made corrections and rebased onto develop due to complexity.

@droidmonkey droidmonkey merged commit f9eef6d into develop Jun 21, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
TeamCity CI (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the fix/favicon-multiselect branch Jun 24, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.