Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

[Regression] file transfers widget is too small when images are transferred #3042

Closed
zetok opened this issue Mar 23, 2016 · 9 comments
Closed
Labels
C-bug The issue contains a bug report M-ui Affected Module: User Interface

Comments

@zetok
Copy link
Contributor

zetok commented Mar 23, 2016

Brief description: Images in file transfer widget aren't displayed properly any longer, and they're visible "outside" of the file transfer widget

Operating System: Gentoo x86_64
qTox version: 252c9c8
Qt: 5.5.1

Reproducible: Always

Steps to reproduce:

  1. Send/receive picture
  2. Look at left corners of the file transfer widget

Observed Behavior:
Visible corners of pictures, file transfer widget is smaller than it is supposed to be.

Expected Behavior:
Corners of pics aren't supposed to be visible, widget shouldn't be that small.

Additional info:
Before regression:
before

Currently:
regression

@zetok zetok added C-bug The issue contains a bug report help wanted M-ui Affected Module: User Interface labels Mar 23, 2016
@ProMcTagonist
Copy link
Contributor

Not sure what commit removed their padding but it should be reverted

@zetok
Copy link
Contributor Author

zetok commented Mar 29, 2016

git bisect:
good: v1.2.4
bad: 791d871

857dfbc is the first bad commit

commit 857dfbcd4c1ecdab2b35c5bee8974bdcdef99558
Author: Vincas Dargis <vindrg@gmail.com>
Date:   Sun Feb 14 17:20:11 2016 +0200

    Open downloaded image with mouse click on thumbnail

:040000 040000 618a3c899589dc68b88f5f64a30e666a7f4417b4 9e391a2b97e3c09870a9386f1e7023459c63bafe M  src

Introduced in #2953.

With 857dfbc reverted issue is gone.

cc @Talkless

Edit: image with proper file transfer widget:
correct widget

@Talkless
Copy link
Contributor

Oh damn.. I see now.

I was using picture with white background while developing, so I failed to see that strange layout problem.

Gonna try to make take 2.

Thanks for report.

@Talkless
Copy link
Contributor

Talkless commented Apr 2, 2016

I need help with Git.

How could I "replay" these bad change on top of current head to start work on improved version?

If I do just git checkout -b preview_click_v2 857dfbcd4c1ecdab2b35c5bee8974bdcdef99558, then git diff show nothing, as it is already commited.

@zetok
Copy link
Contributor Author

zetok commented Apr 2, 2016

@Talkless well, the bad change is still there, so it doesn't need to be replayed. What you probably would want to do, is to revert the change, and from there add functionality again without the bug.

to revert without committing any changes:

git show 857dfbcd4c1ecdab2b35c5bee8974bdcdef99558 | patch -p1 -R

and from there you can start adding stuff from 857dfbc, and check what causes the bug.

@Talkless
Copy link
Contributor

Talkless commented Apr 2, 2016

the bad change is still there, so it doesn't need to be replayed

Oh OK so I just make a fix on top.

@Talkless
Copy link
Contributor

It looks like that Qt::KeepAspectRatioByExpanding makes picture overflow from QPushButton area.

Behaviour is much different compared to QLabel...

Still working for a solution.

@Talkless
Copy link
Contributor

Here's how it looks currently in my branch.

Tested with various cases (square, horiz/vert rectangles, thinner-than-icon rectangles, overall smaller images):
image_icon_fix

Had to implement scaleCropIntoRect() helper.

I have also made sure, that images would not be upscaled if some dimension is smaller than icon (60px currently, minus border).

Maybe whole background should be white instead of just borders (in case of small images) ?

Comments?

@zetok
Copy link
Contributor Author

zetok commented Apr 13, 2016

Looks great :)

Maybe whole background should be white instead of just borders (in case of small images) ?

Dunno, green seems fine to me. I guess that it's fine to keep it, unless someone will complain..?

Talkless added a commit to Talkless/qTox that referenced this issue Apr 15, 2016
Introduced in 857dfbc

Regression was due to fact that QPushButton allows icon to overflow.
This patch does:
1. Scale and crop icon to fit into button.
2. Avoid upscaling small images.
3. Refactor FileTransferWidget::showPreview() to load image from file
   only once.

Closes qTox#3042
Talkless added a commit to Talkless/qTox that referenced this issue Apr 17, 2016
Introduced in 857dfbc

Regression was due to fact that QPushButton allows icon to overflow.
This patch does:
1. Scale and crop icon to fit into button.
2. Avoid upscaling small images.
3. Refactor FileTransferWidget::showPreview() to load image from file
   only once.

Closes qTox#3042
Talkless added a commit to Talkless/qTox that referenced this issue Apr 17, 2016
Introduced in 857dfbc

Regression was due to fact that QPushButton allows icon to overflow.
This patch does:
1. Scale and crop icon to fit into button.
2. Avoid upscaling small images.
3. Refactor FileTransferWidget::showPreview() to load image from file
   only once.

Closes qTox#3042
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C-bug The issue contains a bug report M-ui Affected Module: User Interface
Projects
None yet
Development

No branches or pull requests

3 participants