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

Change "Add new torrent" dialog to horizontal layout #10600

Merged
merged 1 commit into from May 15, 2019

Conversation

Projects
None yet
5 participants
@Chocobo1
Copy link
Member

commented May 8, 2019

This is adopted from PR #9591, I reverted some (many actually) changes and also improved a bit.
Main difference to PR #9591 are:

  • It doesn't add additional widgets (clickable labels, checkbox word warps), all widgets are from stock, IMO it is inappropriate to introduce them in layout-change PR.
  • Avoid setting widgets attributes explicitly when it is the same as the default.
  • Correctly remember the horizontal splitter state.
  • Follow previous settings layout closely, I don't think qbt should 100% copy utorrent's layout and reverted some questionable widget placements.

Screenshots:
Old: old
New: new2
Previous attempts: 1

@Chocobo1 Chocobo1 added this to the 4.2.0 milestone May 8, 2019

@Chocobo1 Chocobo1 requested review from sledgehammer999 and glassez May 8, 2019

@glassez
Copy link
Member

left a comment

Didn't look at .ui too closely. The rest LGTM.

@thalieht

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Could you add spacers on the right of sequential and first/last checkboxes to avoid moving them around when resizing horizontally?
Also i don't know if you want to do something about it but #9591 (comment) happens here. Personally i see nothing wrong with it but it seems others feel otherwise.
And lastly, the "Do not show again" checkbox seems in the way when there are metadata (may just be personal preference):
Currently:
Capture
Maybe swap?
Capture2

I was thinking (out of the scope of this PR) that now that the list widget is always shown, maybe move the progressbar and the "Downloading metadata" label inside the list or something of that nature? Maybe using a qstackedwidget like in the searchwidget.ui (to display "there are no plugins installed").

@glassez

This comment has been minimized.

Copy link
Member

commented May 8, 2019

I was thinking (out of the scope of this PR) that now that the list widget is always shown, maybe move the progressbar and the "Downloading metadata" label inside the list or something of that nature? Maybe using a qstackedwidget like in the searchwidget.ui (to display "there are no plugins installed").

👍

@Chocobo1 Chocobo1 force-pushed the Chocobo1:newtorrentdlg branch from fa28ba1 to 3a9cf29 May 9, 2019

@Chocobo1

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Could you add spacers on the right of sequential and first/last checkboxes to avoid moving them around when resizing horizontally?

OK, done.

Also i don't know if you want to do something about it but #9591 (comment) happens here. Personally i see nothing wrong with it but it seems others feel otherwise.

Fixed.

And lastly, the "Do not show again" checkbox seems in the way when there are metadata (may just be personal preference):

Done.

maybe move the progressbar and the "Downloading metadata" label inside the list or something of that nature?

I don't mind the change but maybe after this PR.

@sledgehammer999

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Don't merge yet. Wait a few days. I'll try to review it too.

@Chocobo1 Chocobo1 dismissed stale reviews from glassez and thalieht via 8738ebe May 13, 2019

@Chocobo1 Chocobo1 force-pushed the Chocobo1:newtorrentdlg branch from 3a9cf29 to 8738ebe May 13, 2019

@Chocobo1 Chocobo1 force-pushed the Chocobo1:newtorrentdlg branch from 8738ebe to 62c7209 May 14, 2019

@sledgehammer999

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Should we consider this for backport? I know that 4.1.x is in a state of "fixes" only. But, as a frequent user(me), the current layout with the tiny filelist has gotten on my nerves.
The code changes of this PR aren't complex, so we shouldn't have any serious bug if we backport.
@glassez @Chocobo1 what do you think?

@glassez

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Should we consider this for backport?

I have no strict preferences in this regard. But if you decide to backport it, I would advise not to rush it, but first test it in branch 4.2 for some time.

@Chocobo1 Chocobo1 merged commit dca0556 into qbittorrent:master May 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Chocobo1 Chocobo1 deleted the Chocobo1:newtorrentdlg branch May 15, 2019

@Chocobo1

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Should we consider this for backport?

Cherry-picked into #10615.

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.