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 address #20118

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

Display external address #20118

wants to merge 8 commits into from

Conversation

OdinVex
Copy link

@OdinVex OdinVex commented Dec 10, 2023

This adds an external address display label to the GUI and WebUI. Both work showing the last detected address. Systems with multiple addresses will need to be addressed at a later date. This will help an absolute ton of people using qBittorrent via Docker or with single-address setups.

It would be nice to keep track of IPv4 vs IPv6 and to show both. This is just to get it in there and it works.

This a re-submit for closed Pull Request #20096 (unable to re-open).

OdinVex and others added 3 commits December 6, 2023 23:16
Adds an external address display label to the GUI and WebUI. Both work showing the *last detected* address. Systems with multiple addresses will need to be addressed at a later date. This will help an absolute ton of people using qBittorrent via Docker or with single-address setups.

Signed-off-by: Odin Vex <44311901+OdinVex@users.noreply.github.com>
Remove unnecessary copy-pasta re-cast.

Co-authored-by: Vladimir Golovnev <glassez@yandex.ru>
Added statusbar separator, conformed which separator belonged where.
@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

I'll adjust in time, I think this was older tinkering.

Edit: Hm, git clone built fine and ran fine, no adjustments. (cmake .; make)
Edit: Added override, must've forgot. This too compiles, should address the macOS fail. Could someone else peek at it, I shouldn't code so early.

@stalkerok
Copy link
Contributor

stalkerok commented Dec 11, 2023

Screenshots:
GUI: 2023-12-11_203555 2023-12-11_204031
WebUI: 2023-12-11_204316 2023-12-11_204141
Didn't find any problems. But it's still better to use "IP" instead of "Address" to save space, in case in the future someone (maybe you) wants to add a display of both ipv4 and ipv6 address at the same time, or even all possible addresses. Or any other information. IMHO

@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

Didn't find any problems. But it's still better to use "IP" instead of "Address" to save space, in case in the future someone (maybe you) wants to add a display of both ipv4 and ipv6 address at the same time, or even all possible addresses. Or any other information.

I mentioned in the original PR that you (the general you, as in qBittorrent team/developers/whoever it may concern) can decide what language to use. When I see IP I see Internet Protocol, not Address, but I've been developing since 2004 in areas that include developing protocols and it just looks weird to my mind seeing "External Internet Protocol" when I'd need to refer to an address. Again though, I don't care which way it goes in your (general your) project, anyone pick. I'm running around on 28" 4K high-DPI setups, so I don't have space to worry about. *shrug* Personally I'd rather the backend kept track of the last externally-detected addresses via type so it could end up "External Address(es): IPv4, IPv6*" where an asterisk denotes which was detected most recently, somewhat adapting and opportunistic in displaying more, maybe even a popup menu showing a small 'mini-exec' log of addresses detected with timestamp (or enabling the View for the log and simply preset the filter to detected IPs...easiest and most 'integrated' in my opinion, less UI to manage or clutter and easily implemented).

Edit: I might take some time later today to implement separate IPv4/IPv6 addressing and displaying both using 'Addresses' (or only one if only one exists and using 'Address' or whatever the project ends up deciding).

Edit: Cleanly refactoring to support both IP address types is problematic because other functions rely on not making a difference between them, such as isReannounceWhenAddressChangedEnabled (see handleExternalIPAlert for more info). I haven't had any time to grasp the code, would that function matter or care about which type or is it safe to just go ahead and let it through regardless of IP address because it's still an externally detected change?

@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

I've implemented it all but I've come across a formatting situation. Do you want a space between IPv4 and IPv6 addresses or a comma and space or what? Edit: Went with a single space. Tested GUI and WebUI, currently working on my end.

@stalkerok
Copy link
Contributor

comma and space

would be a good option.

Signed-off-by: Odin Vex <44311901+OdinVex@users.noreply.github.com>
@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

comma and space

would be a good option.

I didn't see the message until after commit, my bad. Comma and space would require keeping track of whether both are being shown or just one and adjusting so, was easier to use trim at the time.

@stalkerok
Copy link
Contributor

maybe even a popup menu showing a small 'mini-exec' log of addresses detected with timestamp

A small information window when hovering over an address with all available IP addresses would be a good option (the same as when hovering over "connection status", for example).

@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

maybe even a popup menu showing a small 'mini-exec' log of addresses detected with timestamp

A small information window when hovering over an address with all available IP addresses would be a good option (the same as when hovering over "connection status", for example).

qBittorrent currently doesn't keep track of addresses, merely the last detected of each type. The backend would need some further tinkering to do that. I don't do enough web development with JavaScript (I personally despite JS) to add a popup, but I can look at it. Edit: Perhaps that popup should just show the log filtered for detected IP Addresses?

@stalkerok
Copy link
Contributor

Perhaps that popup should just show the log filtered for detected IP Addresses?

Perhaps I just don't really understand how good or bad it would look.

@OdinVex
Copy link
Author

OdinVex commented Dec 11, 2023

Perhaps that popup should just show the log filtered for detected IP Addresses?

Perhaps I just don't really understand how good or bad it would look.

Edit: I haven't pushed it yet but I've refactored the code to show (and send for WebUI) both IPv4 addresses and IPv6 addresses such as 'External Address(es): Detecting' -> 'External Address: 0.0.0.0' -> 'External Address: f:f:f:f' ->'External Addresses: 0.0.0.0, f:f:f:f'. The addresses past detected are available for someone to write a popup (or something) if desired. All strings allow for translation, of course. I wonder, should we mark the last detected one via asterisk in cases with multiple addresses? (Ex: 'External Addresses: 0.0.0.0, f:f:f:f*') It might be helpful for someone and is easily done.

Edit: Instead of a popup, I'd say just call the View->Log.onclick, call the Execution Log->onclick, set the filterTextInput text to 'Detected external IP.' and set 'Information Messages' to ticked in the drop-down. It'd show IP as well as when they were last used, less UI to develop. That'd work for the GUI and WebUI, too.

Signed-off-by: Odin Vex <44311901+OdinVex@users.noreply.github.com>
@OdinVex
Copy link
Author

OdinVex commented Dec 14, 2023

I decided to reuse elements of the UI. The label at the bottom was converted into a button similar to setting download/uploads speeds. When it's clicked it'll open up the Execution Log and add Information Messages to the filter list to show messages as appropriate. The GUI doesn't currently have a text input means of filtering the log, but at least they'll be shown (Informational type). The WebUI is pre-set with the translatable text and works as well. Try it out, see if it needs tweaking. If you want to call things 'Internet Protocol(s)' instead of addresses be my guest.

Edit: I felt this approach to reuse and reinforce use of the Execution Log as a means of information conveyance was best. It beats adding more UI that would need maintenance and simply works. Every user-facing string is translatable via TR and QBT_TR, I paid attention to make sure I used that 7 strings total including the transitory-state strings.

…or detecting external IP addresses

Signed-off-by: Odin Vex <44311901+OdinVex@users.noreply.github.com>
@OdinVex
Copy link
Author

OdinVex commented Dec 14, 2023

Except one string, forgot that one, added. 😅

@glassez glassez changed the title Add initial external address display to gui and webui (Resubmit of #20096) Add initial external address display to GUI and WebUI Dec 15, 2023
@glassez glassez changed the title Add initial external address display to GUI and WebUI Display external address Dec 15, 2023
src/base/bittorrent/session.h Outdated Show resolved Hide resolved
@@ -86,6 +86,15 @@ StatusBar::StatusBar(QWidget *parent)
m_upSpeedLbl->setStyleSheet(u"text-align:left;"_s);
m_upSpeedLbl->setMinimumWidth(200);

m_lastExternalAddressesLbl = new QPushButton(this);
m_lastExternalAddressesLbl->setText(tr("External Address(es): Detecting"));
Copy link
Member

Choose a reason for hiding this comment

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

How about changing "Detecting" to "N/A"?
Not all users have such large monitors as you, and among those who do, not all have the desire to stretch the qBittorrent window to the full screen and shake their heads left and right to view it.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, the IP address will still be longer. But in any case, all we can say about this is that the information about external address is currently "not available". IMO, "Detecting" looks speculative.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know how that would differentiate between "Not available...yet" and other states such as failed, but I do agree about 'Detecting' sounding speculative. Perhaps more information should be communicated to the frontend from the means that qBittorrent uses to detect. "Detecting/Detection failed" etc? As for how much space is used...at this rate it's looking like you want just external addresses printed and nothing else which, quite frankly, leaves me baffled as to what the information would be.

Copy link
Author

Choose a reason for hiding this comment

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

Then again, some people use torrent clients on closed networks (so local IP address(es)), so maybe it can make sense.

src/gui/statusbar.cpp Outdated Show resolved Hide resolved
src/gui/statusbar.cpp Outdated Show resolved Hide resolved
@glassez
Copy link
Member

glassez commented Dec 15, 2023

If you want to call things 'Internet Protocol(s)' instead of addresses be my guest.

We don't want to call things 'Internet Protocol(s)' instead of addresses. We do want to write IP as a shortcut of IP address. I can't figure out what the problem is. A similar thing (called "metonymy") is used everywhere (just look around, it's strange that you haven't come across it). We say "IP" instead of "IP address", "E-mail" instead of "E-mail address" whenever the context allows us to do it unambiguously.

@glassez
Copy link
Member

glassez commented Dec 15, 2023

When it's clicked it'll open up the Execution Log and add Information Messages to the filter list to show messages as appropriate.

Is it a really good idea to do it this way?
Firstly, it is doubtful to implicitly enable something that was previously explicitly disabled by the user.
Secondly, the log can be filled with a lot of messages, so it obviously won't seem intuitive to the user to see it.

@glassez
Copy link
Member

glassez commented Dec 15, 2023

Personally, I would limit myself (at least to begin with) to something minimalistic like Deluge does. Just IP: 123.456.7.8 or maybe IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501. Details can be shown in tooltip. Or, if it is really necessary to see more details/statistics, we can later add "External address info" dialog that is displayed when you click on appropriate button on status bar.

@stalkerok
Copy link
Contributor

or maybe IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

I don't think that's a good idea, a , separator like that would save some space.
IP: 123.456.7.8, 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501
and
IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

Details can be shown in tooltip.

👍

@glassez
Copy link
Member

glassez commented Dec 15, 2023

or maybe IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

I don't think that's a good idea, a , separator like that would save some space. IP: 123.456.7.8, 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501 and IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

To clarify. I meant two separate items of status bar.
Personally I would keep only addresses w/o IP etc. That would be enough. If not, then someone could look (once) at the details in the tooltip.

@OdinVex
Copy link
Author

OdinVex commented Dec 15, 2023

We don't want to call things 'Internet Protocol(s)' instead of addresses. We do want to write IP as a shortcut of IP address. I can't figure out what the problem is. A similar thing (called "metonymy") is used everywhere (just look around, it's strange that you haven't come across it). We say "IP" instead of "IP address", "E-mail" instead of "E-mail address" whenever the context allows us to do it unambiguously.

I say e-mail address and IP address. *shrug*

Is it a really good idea to do it this way? Firstly, it is doubtful to implicitly enable something that was previously explicitly disabled by the user. Secondly, the log can be filled with a lot of messages, so it obviously won't seem intuitive to the user to see it.
...
Personally, I would limit myself (at least to begin with) to something minimalistic like Deluge does. Just IP: 123.456.7.8 or maybe IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501. Details can be shown in tooltip. Or, if it is really necessary to see more details/statistics, we can later add "External address info" dialog that is displayed when you click on appropriate button on status bar.

That's what showing the Execution Log was to do. It simply re-used the pre-existing UI and filters to just the relevant messages (WebUI can filter by text which makes it much more useful, GUI can only filter by message type and is less useful).

Not sure if this is known, but this doesn't have to be perfect to get such a useful feature implemented. It doesn't harm anything except at most screen-space. On a side note, I disable the Search and RSS but they re-enable themselves and are useless to me. If they intentionally click on the addresses then they intentionally want more information, I don't see any harm in showing the Execution Log tab when they specifically asked for information/details. Fancy conveyance was 'immediately' irrelevant development to simply getting the functionality out there.

…l IP addresses

Co-authored-by: Vladimir Golovnev <glassez@yandex.ru>
@@ -5793,15 +5803,27 @@ void SessionImpl::handleListenFailedAlert(const lt::listen_failed_alert *p)

void SessionImpl::handleExternalIPAlert(const lt::external_ip_alert *p)
{
const QString externalIP {toString(p->external_address)};
QString externalIP {toString(p->external_address)};
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like you need to remove the const:

Suggested change
QString externalIP {toString(p->external_address)};
const QString externalIP {toString(p->external_address)};

Copy link
Author

Choose a reason for hiding this comment

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

I thought I added that back with the latest commit.

Comment on lines +85 to +86
const QString KEY_TRANSFER_LAST_EXTERNAL_ADDRESS_V6 = u"last_external_address_v6"_s;
const QString KEY_TRANSFER_LAST_EXTERNAL_ADDRESS_V4 = u"last_external_address_v4"_s;
Copy link
Member

Choose a reason for hiding this comment

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

Wrong order, keys are sorted alphabetically here.

Copy link
Author

@OdinVex OdinVex Dec 16, 2023

Choose a reason for hiding this comment

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

... I didn't see anything that required it.

Comment on lines +847 to +848
let last_external_address_v4 = serverState.last_external_address_v4;
let last_external_address_v6 = serverState.last_external_address_v6;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let last_external_address_v4 = serverState.last_external_address_v4;
let last_external_address_v6 = serverState.last_external_address_v6;
const last_external_address_v4 = serverState.last_external_address_v4;
const last_external_address_v6 = serverState.last_external_address_v6;

Copy link
Author

Choose a reason for hiding this comment

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

I followed the previous conventions used by qBittorrent for other variables used similarly.

Comment on lines +851 to +853
if (last_external_address_v4 !== "" || last_external_address_v6 !== "")
{
if (last_external_address_v4 !== "" && last_external_address_v6 !== "")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (last_external_address_v4 !== "" || last_external_address_v6 !== "")
{
if (last_external_address_v4 !== "" && last_external_address_v6 !== "")
if ((last_external_address_v4 !== "") || (last_external_address_v6 !== ""))
{
if ((last_external_address_v4 !== "") && (last_external_address_v6 !== ""))

last_external_addresses = 'QBT_TR(External Address: %1%2)QBT_TR[CONTEXT=HttpServer]';
}

$('freeSpaceOnDisk').set('html', 'QBT_TR(Free space: %1)QBT_TR[CONTEXT=HttpServer]'.replace("%1", window.qBittorrent.Misc.friendlyUnit(serverState.free_space_on_disk)));
Copy link
Member

Choose a reason for hiding this comment

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

copy paste error?

Copy link
Author

@OdinVex OdinVex Dec 16, 2023

Choose a reason for hiding this comment

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

Yeah. -_-; Too tired to address right now, will do later.

if (last_external_address_v4 !== "" || last_external_address_v6 !== "")
{
if (last_external_address_v4 !== "" && last_external_address_v6 !== "")
last_external_addresses = 'QBT_TR(External Addresses: %1, %2)QBT_TR[CONTEXT=HttpServer]';
Copy link
Member

@Chocobo1 Chocobo1 Dec 16, 2023

Choose a reason for hiding this comment

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

Although unlikely, I would still suggest using other character for replacement specifier. Maybe $1, $2
See: https://en.wikipedia.org/wiki/IPv6_address#Scoped_literal_IPv6_addresses_(with_zone_index)

Copy link
Author

Choose a reason for hiding this comment

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

I didn't think any device was allowed to start with a number, so I didn't think there any chance of running into %[0-9] with %[0-9] being some interface. I could be wrong but I thought no device could start with [0-9].

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, according to the linked article, Windows will use %[0-9] format:

The latter (using an interface number) is the standard syntax on Microsoft Windows ...

Copy link
Author

@OdinVex OdinVex Dec 16, 2023

Choose a reason for hiding this comment

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

For the WebUI, I believe it should work to do this in reverse-order. Ex:

$('externalAddresses').set('html', last_external_addresses.replace("%2", last_external_address_v6).replace("%1", last_external_address_v4));

Similarly I think the GUI should do the same (reverse order) and always specifically replace the first occurrence only. Edit: I'm away from rig for the day.

Copy link
Author

@OdinVex OdinVex Feb 2, 2024

Choose a reason for hiding this comment

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

I forgot to ask, does qB supply those interfacings or does qB just supply the IP address? If it's IP address then we don't need to account for which interface (unless interface is added to IP addresses). My only test case for this info has been Linux.

Copy link
Author

Choose a reason for hiding this comment

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

I must've been thinking about another client then. Still, there isn't any way for a peer or tracker to know which interface the client is using, so the entire point seems moot.

Copy link
Member

Choose a reason for hiding this comment

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

Still, there isn't any way for a peer or tracker to know which interface the client is using,

Sorry, I don't know much about IPv6. What exactly are you talking about when you mention the interface? About the zone identifier, e.g. %eth0?

Copy link
Author

@OdinVex OdinVex Feb 9, 2024

Choose a reason for hiding this comment

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

Somewhere on this long thread someone mentioned something about not using % in Qt API calls because the IPv6 address might have the zone identifier reported and it'd break it but they're assuming that qBittorrent includes that information...when it gets that address from a remote server (peer/tracker/site). The remote endpoint wouldn't have a clue about which interface qBittorrent is using and won't report that. Edit: There was also suggestion around modifying the JavaScript implementation of that string, I can't remember what it was. I've got too much to do and qBittorrent isn't a high priority. The initial src offering was fine but there's no way to please everyone, so I've stuck to my ViolentMonkey script which does exactly what I want.

Edit: Too long a thread, unable to search and follow it anymore hardly.

Copy link
Member

Choose a reason for hiding this comment

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

the IPv6 address might have the zone identifier reported

IIRC, Zone ID is "locally significant" (enables us to define out which interface we want to send some traffic) and there is no point to send it to remote host as part if its "external address". But we still can't be sure that it isn't sent in such a way (with Zone ID) so the question is how is it handled by libtorrent, i.e. can it provide we "external address" that mistakenly contains such Zone ID.

Copy link
Author

Choose a reason for hiding this comment

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

Someone familiar with libtorrent's API should weigh in. I resorted to a VM script, so I'm fine and qBittorrent can do with this PR as they want.

@stauffenberg2020
Copy link

Can a country flag be shown next to the IP address?

@OdinVex
Copy link
Author

OdinVex commented Feb 2, 2024

Can a country flag be shown next to the IP address?

Not sure why you'd want that but probably, qB already has the capability to show peers so it'd just be a matter of adding it. For now I'm just using my Violentmonkey script.

@stauffenberg2020
Copy link

Not sure why you'd want that but probably,

I use qB in Docker container attached to a macvlan network. And this macvlan network is encrypted through a VPN. I always want to make sure that the encryption is working properly, i.e. qB is always connected through VPN to the external network. Hope its clear.

@OdinVex
Copy link
Author

OdinVex commented Feb 2, 2024

Not sure why you'd want that but probably,

I use qB in Docker container attached to a macvlan network. And this macvlan network is encrypted through a VPN. I always want to make sure that the encryption is working properly, i.e. qB is always connected through VPN to the external network. Hope its clear.

That doesn't explain at all why a country flag would be what you want. You'd see the external IP address. As for the reason for showing an IP address...same, doing the whole VPN deal.

@stauffenberg2020
Copy link

That doesn't explain at all why a country flag would be what you want. You'd see the external IP address. As for the reason for showing an IP address...same, doing the whole VPN deal.

Well, Somewhat true. I would always choose a VPN from a different geographical region than my current region.

The point is, I want to see a (red/green) flag/icon that my qB container is connected through VPN and not exposing my ip publicly. Since the docker container is connected through macvlan network, it cannot find out what my public ip is, unless the vpn service is down.

Hence, if I see a country flag other than my current region - I will add downloads without hesitation.

@OdinVex
Copy link
Author

OdinVex commented Feb 2, 2024

Well, Somewhat true.

Not "somewhat true" at all, your response did not actually say why, you simply mentioned something you're doing. Just pointing it out.

@stauffenberg2020
Copy link

Well, Somewhat true.

Not "somewhat true" at all, your response did not actually say why, you simply mentioned something you're doing. Just pointing it out.

Did you understand the need for the feature request now? Or should I explain further in detail?

@OdinVex
Copy link
Author

OdinVex commented Feb 2, 2024

Well, Somewhat true.

Not "somewhat true" at all, your response did not actually say why, you simply mentioned something you're doing. Just pointing it out.

Did you understand the need for the feature request now? Or should I explain further in detail?

Of course I did, but I don't make assumptions, that makes for bad communication and bad reading comprehension. You have a reading comprehension issue. Edit: To very clearly clarify, I do understand what you want, I'm just pointing out that you did not say why when I asked you why.

@stauffenberg2020
Copy link

I do understand what you want

Thanks, I only look forward to the implementation when some of the experts have time to do it, not an English class as I am not a native English speaker and neither I want to be a shakespear or churchill.

@OdinVex
Copy link
Author

OdinVex commented Feb 2, 2024

I do understand what you want

Thanks, I only look forward to the implementation when some of the experts have time to do it, not an English class as I am not a native English speaker and neither I want to be a shakespear or churchill.

I've been a Linux Kernel developer, contributed to mainline even, I am an expert, 20 years now and counting. This had nothing to do with English classes or anything of the sort. You did not actually state why, you simply stated something you do which did not explain why. I'm just going to ignore your rude deflections. You can wait for other experts to add it for all I care now.

@stalkerok
Copy link
Contributor

It's a good idea actually, but it could be implemented in another PR.
@OdinVex, could you implement what has already been suggested here? It would also be nice to have a setting to disable the display of IP address. Or even vice versa, that by default this setting would be disabled, so that it would not show IP address.

@OdinVex
Copy link
Author

OdinVex commented Feb 4, 2024

It's a good idea actually, but it could be implemented in another PR. @OdinVex, could you implement what has already been suggested here? It would also be nice to have a setting to disable the display of IP address. Or even vice versa, that by default this setting would be disabled, so that it would not show IP address.

No one seems to agree about how best to implement. I say show both last IPv4 and last IPv6 detected with a proper string informing people, but apparently that's "too much info taking up too much space". Another thinks the execution log shouldn't be shown even if users click on the address(es) and wants a popup. I never got any answer from a Windows user as to whether qB reports IPv6 addresses with zone info or just the address detected (though this is about external IP address, so I'm not sure why they brought that up to begin with...). I'm not going to waste time just to be told it isn't wanted in the end (the crying about screen real-estate, they can just disable it). No design decision makes everyone happy, hands are tied.

The ingredients are there if people want to tinker. I've implemented what I want with a Violentmonkey script, it works flawlessly and just how I like it. It shows last IPv4 and IPv6 addresses and clicking it shows my execution log filtered to show addresses. It also has a nice "info" icon and no one is fussing about screen real-estate or design of it.

Edit: As for whatever their name's suggestion they can hope someone else implements it. Their problem, not mine.

@stalkerok
Copy link
Contributor

No one seems to agree about how best to implement. I say show both last IPv4 and last IPv6 detected with a proper string informing people, but apparently that's "too much info taking up too much space".

You can implement one of the suggested options, I don't think anyone will mind.

IP: 123.456.7.8, 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501
and
IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

Another thinks the execution log shouldn't be shown even if users click on the address(es) and wants a popup.

You can put this off for another PR, maybe someone will implement it later. But I said already my wishes about this, that the popup would look the same as the "connection status" window.

@OdinVex
Copy link
Author

OdinVex commented Feb 4, 2024

No one seems to agree about how best to implement. I say show both last IPv4 and last IPv6 detected with a proper string informing people, but apparently that's "too much info taking up too much space".

You can implement one of the suggested options, I don't think anyone will mind.

IP: 123.456.7.8, 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501
and
IPv4: 123.456.7.8 | IPv6: 2001:0DB8:3C4D:7777:0260:3EFF:FE15:9501

Another thinks the execution log shouldn't be shown even if users click on the address(es) and wants a popup.

You can put this off for another PR, maybe someone will implement it later. But I said already my wishes about this, that the popup would look the same as the "connection status" window.

So that's where the confusion is... That's is not a popup (an actual 'window'), that is a tooltip... That's already in there..

I'm not changing the string from 'External Address...' in my src, it makes no sense without it. Showing a random address with no indication if it's local interface or external address detected makes no sense. If you want it you add it.

@stalkerok
Copy link
Contributor

So that's where the confusion is... That's is not a popup (an actual 'window'), that is a tooltip... That's already in there..

Oops, yeah, that's what I mean.

@OdinVex
Copy link
Author

OdinVex commented Feb 4, 2024

So that's where the confusion is... That's is not a popup (an actual 'window'), that is a tooltip... That's already in there..

Oops, yeah, that's what I mean.

Yeah, that's already in there. If you hover over it you'll see the same thing you see already, it's a default in QT (but I had not gotten around to showing it in web UI I think, at least in the pushed src.)

Copy link

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label Apr 12, 2024
@OdinVex
Copy link
Author

OdinVex commented Apr 15, 2024

Herpderp unstale.

@github-actions github-actions bot removed the Stale label Apr 16, 2024
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

5 participants