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

Display External IP Address in status bar #21383

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Piccirello
Copy link
Member

@Piccirello Piccirello commented Sep 23, 2024

This change displays the last detected IPv4 and/or IPv6 address(es) in the GUI and WebUI's status bar. This does not yet handle systems with multiple addresses of the same type (e.g. multiple IPv6 addresses).

This is a continuation of #20118, which has been closed. I've reused most of the code (with minor changes) but removed the ability to directly open the Execution Log by clicking on the address label.

GUI (resolution 1280x800):
Screenshot 2024-09-24 at 11 35 32

WebUI:
Screenshot 2024-09-24 at 11 36 01

@Piccirello Piccirello added WebUI WebUI-related issues/changes GUI GUI-related issues/changes labels Sep 23, 2024
@Piccirello Piccirello added this to the 5.1 milestone Sep 23, 2024
@Piccirello Piccirello requested a review from a team September 23, 2024 21:10
@stalkerok
Copy link
Contributor

Past comments: #20118 (comment) ("External Address" to "IP" or "External IP"), #20118 (comment) ("Detecting" to "N/A"), #20118 (comment) (Disable the default display).
And the mentioned advanced features (for other PRs): showing the country flag next to the IP and a tooltip with all detected IPs.

@Piccirello
Copy link
Member Author

I went through all of the past comments and chose not to act on these. More info below.

Past comments: #20118 (comment) ("External Address" to "IP" or "External IP"), #20118 (comment)

I don't feel super strongly about this, but "Address" seems appropriate to use here.

("Detecting" to "N/A"), #20118 (comment)

"Detecting" is more accurate than "N/A", given that a) we're waiting to resolve the IP and b) "N/A" means "not applicable" (this doesn't really apply here). I also disagree with the notion that "Detecting" takes up too much space, as seen in the GUI screenshot above on a low resolution display.

(Disable the default display).

I'm not opposed to adding an option for this, though I'd push for enabling this feature by default.

And the mentioned advanced features (for other PRs): showing the country flag next to the IP and a tooltip with all detected IPs.

Agree that these can be added later if people desire.

@glassez
Copy link
Member

glassez commented Sep 24, 2024

"Detecting" is more accurate than "N/A", given that
a) we're waiting to resolve the IP

IMO, it can be perceived as some kind of active action of qBittorrent, but it is not. Moreover, it will never detect the external address without having active torrents.

b) "N/A" means "not applicable" (this doesn't really apply here).

This statement is speculative. "N/A" can also mean other things, e.g. "not available" which is used in this case.

@glassez
Copy link
Member

glassez commented Sep 24, 2024

I went through all of the past comments and chose not to act on these.

If you didn't figure it out, it was those comments that were the problem.

@Piccirello
Copy link
Member Author

Piccirello commented Sep 24, 2024

I went through all of the past comments and chose not to act on these.

If you didn't figure it out, it was those comments that were the problem.

There were many comments, almost all of which I addressed. These remaining comments seem minor. And if I'm being totally honest, I think it was your tone and manor of interacting with the author that lead to them abandoning the PR. You can come across as very terse.

This statement is speculative. "N/A" can also mean other things, e.g. "not available" which is used in this case.

That's not what it means though.

@stalkerok
Copy link
Contributor

I don't feel super strongly about this, but "Address" seems appropriate to use here.

In the log there are strings Detected external IP. IP: “...”, the same here, but without Detected external IP..

IMO, it can be perceived as some kind of active action of qBittorrent, but it is not.

This statement is speculative. "N/A" can also mean other things, e.g. "not available" which is used in this case.

👍
The external address may never be detected (when using a proxy, for example).

Moreover, it will never detect the external address without having active torrents.

A working DHT is sufficient for detection.

@glassez
Copy link
Member

glassez commented Sep 24, 2024

This statement is speculative. "N/A" can also mean other things, e.g. "not available" which is used in this case.

That's not what it means though.

https://wikipedia.org/wiki/Not_available

@glassez
Copy link
Member

glassez commented Sep 24, 2024

Moreover, it will never detect the external address without having active torrents.

A working DHT is sufficient for detection.

I'm not sure that DHT is queried for anything when there are no running torrents. However, it doesn't matter. I just wanted to emphasize that "external address" is information that is obtained as a side effect, and not as a result of active actions.

@stalkerok
Copy link
Contributor

I'm not sure that DHT is queried for anything when there are no running torrents.

#21296 (comment) In the screenshots you can see that if DHT is working, the external address will be detected (without active torrents).

@Piccirello
Copy link
Member Author

Piccirello commented Sep 24, 2024

This statement is speculative. "N/A" can also mean other things, e.g. "not available" which is used in this case.

That's not what it means though.

https://wikipedia.org/wiki/Not_available

That's not the usage I think of when I hear "N/A", but that's just my bias.

I'll switch from "External Address" to "External IP" and from "Detecting" to "N/A".

https://github.com/qbittorrent/qBittorrent/compare/24aa88a28439bed5682d6d0e11cd65db5ee06cc3..45ceb1465199d04d2da3acd231fbbe8763480304

@Piccirello Piccirello force-pushed the external-ip branch 3 times, most recently from 45ceb14 to bbf5400 Compare September 24, 2024 19:27
@stalkerok
Copy link
Contributor

I'm not opposed to adding an option for this, though I'd push for enabling this feature by default.

Any other opinions? Even if you leave the default display, there should be a checkbox in the options to disable the display.

@sledgehammer999
Copy link
Member

I am questing the usefulness of this. Why isn't the log entry sufficient?
Also, I think it will contribute to UI clutter.

@luzpaz
Copy link
Contributor

luzpaz commented Sep 25, 2024

IMHO, if it's optional (via preferences) then I see no harm in it. There are many requests for this functionality IIRC.

@sledgehammer999
Copy link
Member

Another question: Is this mostly useful to WebUI users? Or do GUI users stand to benefit from it too?

@HanabishiRecca
Copy link
Contributor

Why isn't the log entry sufficient?

E.g. my ISP sometimes drops my static IP, effectively blocking the transfers. And it's not always obvious at the first glance if there is simply no activity or something is broken, it requires reading the log.

Is this mostly useful to WebUI users? Or do GUI users stand to benefit from it too?

I don't see how UI choice matters here. It serves the same purpose for both.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Sep 27, 2024

E.g. my ISP sometimes drops my static IP, effectively blocking the transfers.

Wait. I think I am missing something. If the ISP drops the static IP don't you get assigned another IP? Then why doesn't qbt/libtorrent continue transfers?

In any case, if I am the only member who thinks that it clutter the UI, then disregard me. I will not insist.

@HanabishiRecca

This comment was marked as off-topic.

@sledgehammer999

This comment was marked as off-topic.

@HanabishiRecca

This comment was marked as off-topic.

@HanabishiRecca

This comment was marked as off-topic.

@Piccirello
Copy link
Member Author

It seems folks are currently in agreement that there should be a way to disable this. Is that right? Anything else missing?

@HanabishiRecca
Copy link
Contributor

Yes. Even without clutter take, hiding it should be possible for privacy reasons.

Streamer mode in qBittorrent when?

@stalkerok
Copy link
Contributor

Works fine.

@Piccirello Piccirello requested review from a team and removed request for a team October 8, 2024 22:12
@Piccirello
Copy link
Member Author

Are there any other blockers on this PR?

src/base/bittorrent/sessionimpl.cpp Outdated Show resolved Hide resolved
src/gui/optionsdialog.ui Outdated Show resolved Hide resolved
src/gui/statusbar.cpp Show resolved Hide resolved
src/gui/statusbar.cpp Outdated Show resolved Hide resolved
src/gui/statusbar.cpp Outdated Show resolved Hide resolved
src/gui/statusbar.h Outdated Show resolved Hide resolved
src/webui/api/transfercontroller.cpp Outdated Show resolved Hide resolved
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
@stalkerok
Copy link
Contributor

@Chocobo1 Do you have any other comments? I would test again.

src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/optionsdialog.cpp Outdated Show resolved Hide resolved
src/gui/optionsdialog.cpp Outdated Show resolved Hide resolved
src/gui/optionsdialog.cpp Outdated Show resolved Hide resolved
src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved
src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved
src/webui/www/private/scripts/client.js Outdated Show resolved Hide resolved
@Danny3
Copy link

Danny3 commented Oct 22, 2024

This could be a privacy & security nightmare if it's on by default.
Especially with new Windows like Recall which always captures the screen and might even use OCR to transform everything into text.
But of course normal spyware can also do that.
Or just the users capturing the screen for somehting and forgetting about it.
If they are in a live stream, than that's even worse.

While cool to have, I think it should never be enabled by default!

@luzpaz
Copy link
Contributor

luzpaz commented Oct 22, 2024

Especially with new Windows like Recall which always captures the screen and might even use OCR to transform everything into text.

TBH, that is already a preexisting privacy nightmare.

@Danny3
Copy link

Danny3 commented Oct 24, 2024

Especially with new Windows like Recall which always captures the screen and might even use OCR to transform everything into text.

TBH, that is already a preexisting privacy nightmare.

Yes, but having your IP address shown anywhere in the graphical interface will make the nightmare even worse.
I bet even if you have a dynamic IP would not matter as I bet it will record it every time is changed.
So I bet they could go back into Recall's history to find out what IP you had on day X.

AFAIK at the moment no other program shows the current IP address in its graphical interface.

Now that I'm thinking more about it, not only the Windows user that has Recall installed will have its privacy tarnished, but also the ones that appear in the Peers list.

So in my opinion, it would be best for the privacy of everyone if the IP addresses are not displayed by default both in the status bar and in the Peers tab for people that have Recall installed.

@HanabishiRecca
Copy link
Contributor

HanabishiRecca commented Oct 24, 2024

That's kinda long and complicated way to ask for it to be disabled by default.

I also think it should not be enabled by default. Not even only for privacy reasons, but simply because most users won't need it.

@Piccirello Piccirello force-pushed the external-ip branch 2 times, most recently from a1cf921 to 4d3fdff Compare October 24, 2024 20:17
@Piccirello
Copy link
Member Author

All comments addressed.

Folks seem to feel strongly that this should be disabled by default. No one has expressed a strong opinion that this should be enabled by default, so I've made that change too.

Chocobo1
Chocobo1 previously approved these changes Oct 27, 2024
@stalkerok
Copy link
Contributor

At startup the IP is displayed, but the checkbox is disabled. If you go to the options and change any other setting and apply it, the IP disappears. At the next startup everything repeats.

@HanabishiRecca
Copy link
Contributor

Yeah, we forgot about the initial widget state.

OdinVex and others added 3 commits October 29, 2024 12:51
This change displays the last detected IPv4 and/or IPv6 address(es) in the GUI and WebUI's status bar. This does not yet handle systems with multiple addresses of the same type (e.g. multiple IPv6 addresses).
@Piccirello
Copy link
Member Author

At startup the IP is displayed, but the checkbox is disabled. If you go to the options and change any other setting and apply it, the IP disappears. At the next startup everything repeats.

Good catch. Just put up a fix.

Copy link
Contributor

@stalkerok stalkerok left a comment

Choose a reason for hiding this comment

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

Tested 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI GUI-related issues/changes WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display Public IP Address Somewhere in WebUI
9 participants