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

Free space on disk label in status bar (Closes #6764) #6791

Closed
wants to merge 1 commit into from
Closed

Free space on disk label in status bar (Closes #6764) #6791

wants to merge 1 commit into from

Conversation

KingLucius
Copy link

@KingLucius KingLucius commented May 15, 2017

Currently, this PR gives "Free space: 0B". Works after PR #6785.
Free space calculation done against default save path with a tooltip shows the default save path.

Result:

@KingLucius
Copy link
Author

KingLucius commented May 16, 2017

@LordNyriox the drive of the default save path.
If the default save path is : F:/torrents/ it will calculate the free space of F:/ drive.
We can also give it an display option in #6363 so user can show/hide it.

@zeule
Copy link
Contributor

zeule commented May 16, 2017

Maybe it is better to show free space for a torrent target mount point in a tooltip for the transfers list?

@KingLucius
Copy link
Author

You mean instead of showing the free space in status bar, I make it shows when the user hover over a torrent (row) in transfer list ?

@zeule
Copy link
Contributor

zeule commented May 16, 2017

Yes. Wouldn't it make more sense that way?

@KingLucius
Copy link
Author

Personally, I like it more in status bar because most of users download to the same mount point (different folders, yes), by that way I keep an eye on my "main" save area.

@zeule
Copy link
Contributor

zeule commented May 16, 2017

There are many tools to monitor free disk space. Why do you want to do this via a download manager, which is hidden in system tray most of the time?

@KingLucius
Copy link
Author

To keep an eye on my "main" save area. I don't auto download torrents via RSS. I manually add torrents to qbt.
This indicator gives me hint on the free space that left for download.
I used to use deluge for a while and this indicator was useful to me.
#6764 encourage me to start work on it.

@thalieht
Copy link
Contributor

Maybe it is better to show free space for a torrent target mount point in a tooltip for the transfers list?

Bad idea. That would exclude showing in a tooltip things that actually belong in a tooltip such as the full string of an elided string.

@zeule
Copy link
Contributor

zeule commented May 16, 2017

That would exclude showing in a tooltip things that actually belong in a tooltip....

How? Tooltip can show anything you want.

@thalieht
Copy link
Contributor

Not technically of course. Sure you can use a modifier key for different tooltips or some other condition but that would be unintuitive in this case IMO.

{
QString defaultPath = BitTorrent::Session::instance()->defaultSavePath();
m_freeSpaceOnDiskLbl->setText(tr("Free space: %1").arg(Utils::Misc::friendlyUnit(Utils::Fs::freeDiskSpaceOnPath(defaultPath))));
m_freeSpaceOnDiskLbl->setToolTip(defaultPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a bit more expressive tooltip here.
Something like:
setToolTip(QLatin1String("Free disk space in default save path:") + '\n' + defaultPath)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about ?

setToolTip(tr("Free disk space in default save path:") + '\n' + defaultPath)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This title is drawn over two header lines in the tooltip, maybe we can do
setToolTip(tr("Default save path:") + '\n' + defaultPath)
What do you think?

@Chocobo1
Copy link
Member

Also I prefer moving this widget to the left of DHT widget, let the network related widgets group together.

{
QString defaultPath = BitTorrent::Session::instance()->defaultSavePath();
m_freeSpaceOnDiskLbl->setText(tr("Free space: %1").arg(Utils::Misc::friendlyUnit(Utils::Fs::freeDiskSpaceOnPath(defaultPath))));
m_freeSpaceOnDiskLbl->setToolTip(QString::fromUtf8("<b>") + tr("Default save path:") + QString::fromUtf8("</b><br>") + '\n' + defaultPath);
Copy link
Member

@Chocobo1 Chocobo1 May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setToolTip(QString("<b>%1</b><br>%2").arg(tr("Default save path:"), defaultPath))

@thalieht
Copy link
Contributor

There is a translated string for "Default Save Path:" is there any way to use it here? (writing it in the tr() doesn't work)

@KingLucius
Copy link
Author

@thalieht I think all translatable strings can be translated using
https://www.transifex.com/sledgehammer999/qbittorrent/

@thalieht
Copy link
Contributor

Yes but i mean that since there is already a translated string why not use it if it's possible.

QPushButton *m_connecStatusLblIcon;
QPushButton *m_altSpeedsBtn;
QWidget *m_container;
QHBoxLayout *m_layout;
void updateFreeSpaceOnDisk();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this up, to just below void updateSpeedLabels();

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

QPushButton *m_connecStatusLblIcon;
QPushButton *m_altSpeedsBtn;
QWidget *m_container;
QHBoxLayout *m_layout;
QHBoxLayout *m_layout;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove trailing spaces

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@KingLucius
Copy link
Author

KingLucius commented May 22, 2017

I added WebUI support, Do you think I have to push the new commits as a new PR or just squash it to this PR?

PS: the new changes are independent of this PR, I mean that if @evsh sees that this PR is useless I think he won't mind tracking the free space via WebUI of a headless machine.
I will wait for your call guys.

@Chocobo1
Copy link
Member

I added WebUI support, Do you think I have to push the new commits as a new PR or just squash it to this PR?

New PR, this one is already approved by some of the devs.

@zeule
Copy link
Contributor

zeule commented May 22, 2017

I'm against this feature: unrelated to the app purpose, barely useful and thus waste of resources and screen space. The same in WebUI makes much more sense, because qBt can run on another machine.

Do not have any comments to the code.

@glassez
Copy link
Member

glassez commented May 22, 2017

Yeah, you can still add the display of the current time :)

@KingLucius
Copy link
Author

I think I got what you mean and I think you're totally right. It's quite more useful to have this in WebUI.

I think we can close this and think about #6829

@KingLucius
Copy link
Author

So, done here. Close it ?

@KingLucius KingLucius closed this May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants