[3.3.0.dev1] Developer Discussion

[slook]

Matters regarding the nicotine-plus code that are relevant for the future version of Nicotine+ 3.3.0 due for release some time in 2023.

**This is not a conversation thread, and as such, please keep replies in-line and on-topic.**

[mathiascode]

COMPLETED in Nicotine+ 3.3.0.dev5 - Change all GTK widget names to snake case in .ui files

This can be done gradually in a single .ui file at a time. Any reference to the widget needs to be changed in the Python codebase as well (simply searching for the widget ID in the codebase should cover this).

This is also an opportunity to get rid of inconsistencies in the naming (some of them dating back 20 years):

• add missing underscores in existing camel case names, if any, e.g. welcomepage -> welcome_page
• indicate the widget type/purpose in the name, e.g. IgnoreUser -> ignore_user_button
• use complete words, e.g. descr -> description
• dir/directory -> folder

[slook]

I like the way widgets are in CamelCase and code definitions are snake_case , because it helps to differentiate the type of object being referenced. If any plugin developers have widget references in their code then this could break things.

However, if there is a strict convention for the widget names we have to follow, then I understand the advantage of uniformity.

[mathiascode]

I like the way widgets are in CamelCase and code definitions are snake_case , because it helps to differentiate the type of object being referenced.

The second point in my list should make it clear that you're referencing a widget (and also the type of widget). I'd like to follow the PEP 8 naming conventions everywhere, but the widget names stick out like a sore thumb at the moment.

If any plugin developers have widget references in their code then this could break things.

Unfortunate, but the GUI code has evolved a lot in recent months anyway. If we stick to a consistent naming scheme, this change should only be needed once.

[slook]

The second point in my list should make it clear that you're referencing a widget (and also the type of widget)

Ah yes, that makes even more sense.

to follow the PEP 8 naming conventions

Good call.

[mathiascode]

We've almost eradicated PascalCase (yay!), only these preference pages left:

• chats.ui
• downloads.ui
• nowplaying.ui
• search.ui
• uploads.ui
• userinterface.ui

[mathiascode]

General quality of life improvements for Windows version

Attempt to use the Windows version of Nicotine+ and document any bugs that are found. Investigate known issues, such as bad multi-monitor support, and create issue reports with minimal reproducers for the GTK project. Check whether or not these issues are fixed in GTK 4.

[slook]

During Windows testing, try performance of very large file transfers with reference to unresolved discussion #1533 (comment)

For upstream GTK 4 UI issues see helpful bug report #2014

[mathiascode]

I think all known issues have either been reported to GTK, or are covered by existing issues.

[slook]

RESOLVED in Nicotine 3.3.0 - The headless mode does not support the --bindip command line option (Edit: it does)

Network interface binding would only seem to work if it is defined in config. Nothing in the help documentation explains that this feature is lacking so it would be confusing to a user who has a specific network environment and wishes to start the application from their shell.

nicotine-plus/pynicotine/cli/application.py

Line 27 in 0290f33

def __init__(self, core, ci_mode):

It could be desirable to implement the command line option for run_cli() the same as run_gui().

[mathiascode]

Thanks, this is an oversight.

[mathiascode]

Actually, it's not. The IP and port passed to NicotineFrame are unused, since they're passed directly to the core in init.py nowadays.

[slook]

FIXED [only dev affected] - Critical Error Crash after Change Password confirmation dialog gets sent to back behind other windows

Steps to reproduce the bug

Preferences > Network > Change Password > [new_password] > OK > Don't Click OK > Deactivate application window (message box goes behind) > Change Password > Crash

Nicotine+ Version: 3.3.0.dev1
GTK Version: 3.22.11
Python Version: 3.5.3 (default, Nov  4 2021, 15:29:10) [GCC 6.3.0 20170516]

Type: <class 'NameError'>
Value: free variable 'self' referenced before assignment in enclosing scope
Traceback:   File "/home/sli/Git/dev/cmd/pynicotine/gtkgui/widgets/dialogs.py", line 181, in <lambda>
    self.entry.connect("activate", lambda x: self.dialog.response(Gtk.ResponseType.OK))

More info:
c73c402#r73959443 and cbd09ac#commitcomment-74225927 and #2049

[mathiascode]

It might be time to reconsider if log.add_important_info and log.add_important_info should be used as extensively as they currently are.

• "Your password has been changed." This can be a log message visible in the status bar at best. No need to waste an important message on this, since the user requested the change.
• Any messages related to now playing: Log the messages and show them in the status bar, add a section to show the messages in the preferences dialog next to the button to test the feature.
• "Unable to connect to the server. Reason: %s": Add a popup for the INVALIDUSERNAME reason that actually explains what happened and contains a button to change the username, similar to INVALIDPASS. Add a regular log message as fallback for any other reasons given (which should not exist at the moment).
• "Someone logged in to your Soulseek account elsewhere". This popup is annoying since it can appear anywhere in the application at any point. We could probably log this as the last message after being disconnected from the server, so it remains visible in the status bar.
• Admin messages from the server: Should be visible if ever sent, but I don't think the server has sent one in many years. Keep a popup as a fallback here.
• Information about invalid port and network interface can remain as popups.

[slook]

Add a popup for the INVALIDUSERNAME reason that actually explains what happened and contains a button to change the username, similar to INVALIDPASS

I would prefer to utilize the FastConfigure() dialog for this purpose, if we implemented a way to show(page) then the error message can be displayed within the pane in there, before the "account" page (obviously it would be skipped in the normal non-error flow).

Currently in master, there exists a crash when starting the application with an invalid password and auto-connect on startup set, then pressing either button in the OptionDialog() leads to a Critical Error:

Error in sys.excepthook:
Traceback (most recent call last):
  File "/home/user/Git/dev/master/pynicotine/gtkgui/widgets/dialogs.py", line 157, in on_response
    self.callback(self, response_id, self.callback_data)
AttributeError: 'OptionDialog' object has no attribute 'callback'

Clearly something has not initialized yet. This makes dev1 very unstable at the moment for first run users!

[slook]

FIXED in Nicotine+ 3.3.0 - Setup Assistant dialog box is off-center on first run in Xfce

Nicotine 3.2.3.dev1 and Nicotine 3.3.0.dev1 on MX Linux 18.3 (Debian 9) GTK 3.22.11 Xfce 4.12
Nicotine 3.2.7.rc1 and Nicotine 3.3.0.dev4 on MX Linux 21.1 (Debian 11) GTK 3.24.24 Xfce 4.16

[slook]

yes, the Setup Assistant dialog window is now in centre on first run :)

The unrelated issue with on_window_size_changed() being recursively called whilst the main window is restored does still exist, it happens at around 60 times per second. See suggested fix in #2322 -- FIXED

[slook]

Now since 3ef44c8 the Unavailable Shares dialog is off-center when Rescan on Startup is enabled:

The position is based on the restored size of the window, although the position is always from 0,0. In the above screenshot, the maximized state is saved and recalled at startup, but the saved restored position of the un-maximized main window is on the right-hand side of the screen, yet the dialog appears as if it were on the left. Of course, if main window is maximized then it should ideally be in the centre.

It looks like the maximized state is recalled after the dialog is shown.

[mathiascode]

I think the issue is more likely to be a result of commit 520651f. It seems like GTK 3 explicitly sets Gtk.WindowPosition.CENTER_ON_PARENT for message dialogs.

We can probably set this to Gtk.WindowPosition.NONE instead, which is what our regular dialogs use.

[mathiascode]

I think I have a solution that should fix the centering issue on startup, and also retain the default behavior of GTK dialogs in different window managers. Try the latest commits in master, and let me know if they work on your end.

[slook]

Dialogs all centered here :)

[slook]

RESOLVED in Nicotine+ 3.2.3 (regression in dev1) - The Users panel becomes the wrong width when switching to another Chat tab after the Public feed is Auto-joined on startup

Related to recent changes starting with commit a96dc33

The Public feed always seems to be the default Chat tab on start-up, then switching always then shows the panel width at maximum width:

It must be resized for each room at the start of every session. Turning off the Public room feed auto-join avoids the issue.

[mathiascode]

Cannot reproduce on GTK 3.22.30 and 3.24.33.

[slook]

'Remember the previously open chat tab at startup' was On, disabling it made the issue go away, and now I cannot reproduce either, even when re-enabling the remembered tab setting.

[mathiascode]

Fixed in 654e968. This issue happened if you started Nicotine+ unmaximized, connected to the server, maximized the window and switched to chatroom tabs.

[slook]

FIXED in Nicotine+ 3.2.3 - Chat username tags are hyperlinks if the user's username is a web address

The User menu is inaccessible and their online/away/offline status is invisible, if the user has chosen an URL for their username. This practice seems to have become a recent trend.

The quirk happens somewhere in here:

nicotine-plus/pynicotine/gtkgui/widgets/textview.py

Lines 126 to 141 in 2e32129

	if find_urls and ("://" in line or "www." in line or "mailto:" in line):
	# Match first url
	match = self.url_regex.search(line)
	# Highlight urls, if found and tag them
	while match:
	_usertag(buffer, line[:match.start()])
	url = match.group()
	urltag = self.create_tag("urlcolor", url=url)
	self.tag_urls.append(urltag)
	_append(buffer, url, urltag)
	# Match remaining url
	line = line[match.end():]
	match = self.url_regex.search(line)

Furthermore, the links are invalid because the end ] is included in the URL.

Somehow we need to skip this if the match is found at the start of line within the length of username, or move it earlier in the method so that the username tag has priority.

[slook]

@Nachtalb has a solution for this, see #2131

[slook]

IMPROVED in Nicotine 3.2.3 - Change Username and Change Password mechanisms are unreliable and unpredictable

For example, the Setup Assistant allows the username or password to be changed for writing directly into config but without testing if we are core.logged_in or not. A mechanism like what happens in preferences.on_change_password_response() is required here to prevent the user from getting locked out of their account, which they wouldn't become aware of until later restarting the application. We should request to the server to change the password but this doesn't get done, so it looks like the change was actioned successfully.

Whilst off-topic for 3.3.0, it is noteworthy that for a legacy end-user of GTK 3.22.11 that upon later restart a crash could occur in the Invalid Password dialog which bricks the installation see #1879 (comment)

Even if there is no crash in GTK 3.22.30+ the user can easily be locked out of their account (because by then they may have forgotten their previous username/password or wouldn't think to even try it, they would naturally try the username password they most recently entered).

I suggest we do away with the dialog prompts for INVALIDPASS and INVALIDUSERNAME and make the Setup Assistant more capable of handling all account creation and login and password changing tasks better (possibly even creating a core module for it), for example; by being capable of displaying message strings set by the core and having a one-page mode where the "Next" button label changes to "Connect" or "Login" or "Change Password" as appropriate for the current state and what is entered into the dialog. We could set some flags such as is_new_config is_change_login is_change_passw to identify what we're supposed to be doing.

During first run, Change Login or Change Password both need to be handled together as a pair, because a wrong password usually means the username is already taken. For an existing account (not first run), I see no harm in offering both options any. In other words, both of these functions can share the same dialog page, ideally with slightly different message strings, thus, separate dialogs are not required.

"Connect" would be appropriate during first run for the next_label, obviously during first run we must make sure progression to the port_page happens robustly after having validated the account, and only going forward once a connection is successful (currently, it would seem that the misleading UPnP error displayed in the status bar is preventing connection, so a user might go off on the wrong trouble-shooting path thinking that port forwarding is "crucial" in order to establish a connection, when infact this isn't the case, and then later it turns out there is some error about existing user account etc, the user might think this means a connection cannot be established).

Of course there is no support in chat available whilst disconnected, so a new user would probably abandon the project and try a different client that doesn't require "crucial" port forwarding for initial connection. Even though they have simply picked an unavailable username.

As a sidenote, if FastConfigure() had a core module, then this would also eliminate the need for login related message strings to be duplicated into the cli.application as would be currently the case (btw, I suggest 'configure.py' instead of 'setup.py' because this is ambiguous with an unrelated file in the nicotine-plus parent).

If the above suggestion or something similar were to be implemented, then the Preferences > Network Username and Password TextEntry boxes could be done away with, in favor of a button which destroys the Preferences dialog and opens the Setup Assistant dialog onto the account_page with the appropriate message string(s). In this scenario, it would be needed to have the dialog close after hitting the button, and obviously for the changes not to be written into config without notice if the dialog is destroyed by the user on close (X) (the "Previous" button might become "Cancel").

I have tried a couple of quick experiments which are promising but all have various downsides, so this requires larger changes to several modules to implement cleanly (and is probably beyond my capability to do it right) so further discussion is needed before any further work is carried out, and it will be way better if you design and implement these critical core components initially the way you want them.

Sorry, this is long! Perhaps a less radical approach would be better? Any opinions, suggestions or ideas welcome.

[mathiascode]

I think it's better if the setup assistant only prompts for a username and password if they haven't been filled in previously. I've implemented this in cfff221.

It would perhaps be nice if the setup assistant could verify the username and password on first run.

[slook]

I wouldn't know how best to approach such an implementation, but in principle perhaps one possible way that this could be achieved is by running FastConfigure() and it's spawned dialog box in a separate Thread, as such would allow core program execution to continue in the background and fall into a while need_config() loop such that is done with UPnP() in #2050 (which effectively pauses the mapping of the port in order to prevent UPnP error messages from appearing in the status_bar label or console until after the account page of the first run account setup).

        while self.config.need_config():
            # On first run, wait until we have specified a username and password
            time.sleep(1)

            if self.core.shutdown:
                return

Perhaps something similar can be done to hold the execution of connect() in the same fashon, (instead of a return statement). Now that the popup dialogs play together nicely, an idea like this might be lighter weight and easier to maintain, as opposed to having independent connect and login mechanism calls from within the gtkgui.fastconfigure module itself (this would also help ease cli implementation too).

Additionally, we could use this after the next port_page, which would then allow the missing port TextEntry spin boxes to be added, because it would circumvent the need for an application restart to allow setting a custom port range.

Of course threading and while loops also lead to a high risk of unforeseen issues, but for example an issue that I do foresee is the Invalid Password dialog currently directs to the Preferences dialog which is no use in this case, so it would need to directly launch the Change Password dialog instead (infact this would perhaps be a better UX in any case, regardless of the threading idea), or go back to the account_page somehow without spawning the normal dialog prompts.

---

If this turns out not to be viable, then I it could well be more graceful to simply refine the initialization entry points of the relevant modules to fire at the appropriate times.

It makes me wonder, what it would take to include the port range settings into need_config() , maybe it could be split into need_login_config() and need_port_config(), leave the ports set to None at startup for first run and pre-fill the TextEntry boxes with config.defaults on fastconfigure.show()

To attempt the first login, can we just call core.connect() on_switch_page() , and then on Next do the same for UPnP init? I suppose the user would only get one try tho, can we unload the upnp module upon failure? Can we perhaps even unload on demand for port setting change then restart it? If so then this would close #2025 at the same time as our current goals of improving the sequence of login events.

... can we unload the upnp module upon failure?... perhaps even unload on demand for port setting change then restart it?

Maybe unloading the upnp module is not nessassry, if we added something to close() or detach() or shutdown() any existing binding before creating a new one here...

nicotine-plus/pynicotine/slskproto.py

Lines 575 to 579 in 80367f4

	ip_address = self.bindip or ''
	for listenport in range(int(self.portrange[0]), int(self.portrange[1]) + 1):
	try:
	self.listen_socket.bind((ip_address, listenport))

... or created a new unbind_listen_port() method for the purpose?

[slook]

By default, Nicotine+ launches new Player instances for a multi-selection rather than enqueuing tracks

The 'Media Player Command' needs configuring better to make it re-use an instance if one was launched by N+ earlier. Most popular players have an --enque or /add command line option or similar function, but config.defaults do not make use of this.

This seems especially dumb if it is a multi-select operation, as opening 100's of files can easily hang the machine.

Where track enquing is not configured or not possible, we should limit the number of external application instances that we launch during the same 'Send to Player' operation to a reasonable amount.

[slook]

RESOLVED in Nicotine+ 3.3.0

The Private Chat list popover box is too small and cannot be resized

User reports it looks stupid on a massive 4K screen or whatever.

We should make an effort to calculate the size based on the current size of the main window.

[mathiascode]

Yes, we basically need the dialog resize code for popovers as well, so they're still usable on small screens.

[slook]

It is more of a problem with the popovers because the visible area is constrained by the main window viewport size.

[slook]

CANT FIX - International user (PL) reports issue with character encoding

[j_] speaking of N+ I wonder if it's possible to fix issues with character encoding
[j_] no idea what other users use but I get invalid characters instead of proper ones
[S_] i have luck with utf-8 international, utf-16 international
[j_] can you even choose character set in n+?
[S_] older versions for sure

It is true that it is often seen the square representers for unicode characters in chat, unsure if that what the user meant or if it is something to do with filename encoding or suchlike.

It would be interesting to investigate what some of those characters are, how they are entered and how they end up being transmitted/displayed incorrectly.

[mathiascode]

It is true that it is often seen the square representers for unicode characters in chat

Examples of where this happens "often" would be useful. Unless they use Soulseek NS, which doesn't support UTF-8, other clients should work fine.

Edit: since you mentioned square representers, it's likely that you system doesn't display emoji properly, possibly due to age. You can copy and paste them into a search engine to check.

[mathiascode]

There is nothing actionable here on our end.

[mathiascode]

COMPLETED in Nicotine+ 3.3.0 - Use header capitalization in (short) tooltips according to the new GNOME HIG: https://developer.gnome.org/hig/patterns/feedback/tooltips.html

[slook]

DONE in Nicotine+ 3.3.0 - Nicotine+ custom / commands need moving from the GTK GUI into the core

---

---

such that certain commands might potentially be available from the CLI in future, we cannot depend upon the gtkgui.widgets.textentry for the processing of those keyed inputs, and do not need it because Nicotine+ just uses arg.parse and not Gtk to handle the stdin, if desired we can simply pass forward piped commands to a non-unique GWindow instance just the same by using our ordinary methods that aleady exist. But for main program functionality, the user might choose some core N+ commands when running headless in another console as the same user on the same machine.

This could enable for easier management of seed boxes and suchlike. For example, a --rescan command can be piped to the active process and then exit back to the user's shell, thus it would not be necessary to shutdown and restart the client to perform a rescan. Other useful commands to pipe might be /add and /rem and /ban and /unban, possibly others (after carefully considering the security implications of each command we expose to other external programs via the shell).

[mathiascode]

Commands groups and aliases added in mathiascode@0042dc9

Related: #1540 (comment)

I'll restore basic support for the old syntax and deprecate it.

[mathiascode]

I believe my cli-commands-test branch is now more or less ready to port the rest of the old commands.

[slook]

The cli-commands-test branch is currently out of date, can it be merged with latest master?

[mathiascode]

Rebased.

[slook]

Interesting info: https://github.com/eLvErDe/museek-plus/blob/36c3e9bfb491ee5f6a4a31e73ba38c888b482407/mucous/pymucous/MucousHelp.py#L30-L212

  _____  __ __   ____  ____  __ __  ______",\
" /     \|  |  \_/ ___\/  _ \|  |  \/  ___/",\
"|  Y Y  \  |  /\  \__(  <_> )  |  /\___ \ ",\
"|__|_|  /____/  \___  >____/|____//____  >",\
"      \/            \/                 \/",\
"/help          (This Message)         /quit          (Shut down Mucous)",\
"/help connect  (Connection Commands)  /help setup    (Setup Commands)",\
"/help mode     (Mode Commands)        /help user     (User Commands)",\
"/help chat     (Chatroom Commands)    /help ticker   (Ticker Commands)",\
"/help transfer (Transfer Commands)    /help download (Download Commands)",\
"/help browse   (Browse Commands)      /help search   (Search Commands)",\
"/help keys     (Special Keys)"]

[mathiascode]

Remove Apply button from preferences dialog

Should we remove the Apply button from the preferences dialog? It feels like an old element applications don't use anymore (or maybe exclusive to Windows?), and I'm not sure how useful it is. Most applications seem to either save preferences instantly, or opt for a Cancel/OK combination that close the dialog.

Perhaps this would also be an opportunity to replace the old-fashioned OK label with something like Save. Applying changes instantly would require a revamp of the internals, which I'm not ready for right now.

[slook]

It doesn't make any real difference, I don't feel any specific effort is warranted unless other changes need to made anyway.

[slook]

Enhancement idea for Chat Autocomplete: Always show dropdown list for a word starting with the / character

This is useful for accessibility when using the N+ commands to operate the interface.

Currently, the desired functionality can be acheived by setting Autocomplete Minimum Characters to "1", but there are performance and usability issues with doing so, since the list of suggestions is too large for any alphanumeric character.

[slook]

RESOLVED in Nicotine+ 3.2.5 / 3.3.0 - The ".old" files are overwritten too often, so the backup is easily lost

It doesn't seem to be necessary to make a backup during a session, whereas it would be more important to keep the old data from the previous session.

Perhaps the file should only be overwritten once on quit, and certainly not if the file is going to be overwritten with an empty data set.

[slook]

The problem occurs when write_configuration() calls write_file_and_backup():

nicotine-plus/pynicotine/config.py

Line 770 in f7091b9

write_file_and_backup(self.filename, self.write_config_callback, protect=True)

This happens from many places including during startup, which everytime overwrites the old file before saving the new (current) file:

nicotine-plus/pynicotine/utils.py

Lines 609 to 613 in f7091b9

	# Back up old file to path.old
	try:
	if os.path.exists(path_encoded):
	from shutil import copy2
	copy2(path, path_old_encoded)

The logic is flawed since it is possible for an .old file to be overwritten with defaults, and the logic is non-sequitur anyway because overwriting it everytime guarantees that the backup will not contain anything useful, as the desired data will be long gone.

We certainly should not overwrite an old file if need_config. Furthermore probably once on successful quit might be adequate, and again safeguarding to ensure not need_config doesn't overwrite with useless defaults in any case.

[mathiascode]

which everytime overwrites the old file before saving the new (current) file

load_file loads the config.old file if it has to.

it is possible for an .old file to be overwritten with defaults

We should not end up with defaults or corruption in the config file anymore after the recent changes, so I consider this issue resolved.

[slook]

I consider this issue resolved.

Agreed, the .old file mechanism is clearly not intended for the purposes of the user, and there shouldn't be a scenario where the config breaks.

load_file loads the config.old file if it has to.

I am interested to know, what scenario might arise that would cause a need for this to happen?

[slook]

New chat messages are visually glitchy in TextView after the maximum buffer limit is reached

The new message line appears until replaced with another new message.

In this scenario we should purge the oldest lines from the top of the textview buffer instead.

[mathiascode]

But we do purge the oldest lines, or at least the code is supposed to do so.

[slook]

You're right, upon closer observation it looks more like a Gtk bug because it is visually glitchy, and it seems partially related to scrolling, or at least scrolling slightly up and back down again escapes from the erroneous visual state:

2022-07-13.20-39-55.mov

Also when scrolled mid-way, the textview doesn't stay stationary when the buffer it at maximum, so it seems we need to nudge the scroll position down by one to compensate for purged line, perhaps doing so would help to avoid this glitchy behaviour.

nicotine-plus/pynicotine/gtkgui/widgets/textview.py

Lines 151 to 160 in a09c3d6

	if num_lines >= self.max_num_lines:
	# Limit number of lines in textview
	start_iter = buffer.get_start_iter()
	end_iter = buffer.get_iter_at_line(num_lines - self.max_num_lines)
	if GTK_API_VERSION >= 4:
	_position_found, end_iter = end_iter
	self.tag_urls.pop(end_iter, None)
	buffer.delete(start_iter, end_iter)

[mathiascode]

Presumably improved in aeb3111

[slook]

Feature suggest: If a single File item is selected in a transfers view, then "Browse Folder(s)" should become "Browse to Shared File"

We would seem to have the capability to do this, but it is only possible for the containing folder to be browsed. It seems especially dumb when this is set to the default double-click action, which is not user friendly in this case.

This could also present an opportunity to get rid of the multiple(s) item(s) notation at least for this menu item(s), in favour of a more intelligent dynamic menu item(s) label(s):

"Browse to Shared File" (navigate to a single file to be highlighted in a Browse Shares tab)
"Browse to Shared Folder" (selected a folder or multiple files in a single folder to be navigated in a Browse Shares tab)

[slook]

FIXED in Nicotine+ 3.2.3 - Escaping from Chat Find TextEntry after having executed at least one query causes TextView to scroll to top

No automatic scrolling is needed at all so the scroll position should remain where it is, or it should scroll to the bottom.

[slook]

FIXED (dev) - Critical Error on the second time of editing a value in a ListView within a Plugin Settings dialog

1. Preferences
2. Plugins
3. Test Replier
4. Settings
5. select first item
6. Edit
7. Change value
8. OK
9. select another item
10. Edit
11. Change value
12. OK
13. Crash

Nicotine+ has encountered a critical error and needs to exit. Please copy the following message and include it in a bug report:

Nicotine+ Version: 3.3.0.dev1
GTK Version: 3.22.11
Python Version: 3.5.3 (default, Nov  4 2021, 15:29:10) 
[GCC 6.3.0 20170516]

Type: <class 'AttributeError'>
Value: 'EntryDialog' object has no attribute 'entry'
Traceback:   File "/home/user/Git/dev/master/pynicotine/gtkgui/widgets/dialogs.py", line 157, in on_response
    callback(self, response_id, callback_data)
  File "/home/user/Git/dev/master/pynicotine/gtkgui/widgets/dialogs.py", line 558, in on_edit_response
    value = dialog.get_entry_value()
  File "/home/user/Git/dev/master/pynicotine/gtkgui/widgets/dialogs.py", line 236, in get_entry_value
    return self.entry.get_text()

Edit: for some reason I cannot replicate the crash, maybe I am mistaken..

[mathiascode]

The edit dialog in the plugin settings is currently busted in 3.3.0.dev1 in any case. Changes are not saving at the moment.

[mathiascode]

Edit: for some reason I cannot replicate the crash, maybe I am mistaken..

Seems similar to the other dialog crashes you've reported in the past. Maybe something isn't completely right in older GTK versiions.

[slook]

Yes I just noticed the changes didn't save.

I will come back to testing this again when I have the Gtk 3.22.30 box up and running.

[slook]

FIXED in Nicotine+ 3.2.3 - TrayIcon set_download_status() updates an invisible label unnecessarily

It would only be needed to update the menuitem labels whilst the TrayIcon menu is open, and only while transfers are in progress. Also set_upload_status(). Or the update interval could be made less often than the status_bar widgets.

At long idle without any interaction with the trayicon menu:

[mathiascode]

It would only be needed to update the menuitem labels whilst the TrayIcon menu is open

That's not possible, there's no signal for opening the menu.

0342a37 should help

[slook]

No it doesn't seem to make any difference, set_download_status remains at the top during idle, and is more resource hungry than busy chat and incoming search activity (although perhaps this just proves how slick the rest of the application is!).

If the speed is equal to 0 then it doesn't need updating more than one time. Perhaps some flag is needed that indicates the label string was already set to "0 B/s" once already.

Also human_speed() would benefit from a similar optimization if speed == 0: return "0 B/s" # set string explicitly to avoid unit calculation, because human_speed() can be skipped in this case.

[mathiascode]

No it doesn't seem to make any difference, set_download_status remains at the top during idle, and is more resource hungry than busy chat and incoming search activity (although perhaps this just proves how slick the rest of the application is!).

In that case, I assume there must be download activity that updates it every second?

If the speed is equal to 0 then it doesn't need updating more than one time. Perhaps some flag is needed that indicates the label string was already set to "0 B/s" once already.

That's what my commit did. Could you put in some logging to verify?

Also human_speed() would benefit from a similar optimization if speed == 0: return "0 B/s" # set string explicitly to avoid unit calculation, because human_speed() can be skipped in this case.

This sounds like premature optimization, unless you actually have output that shows that this is a problematic area. No calculations take place when the speed is 0:

nicotine-plus/pynicotine/utils.py

Lines 383 to 387 in 716e997

	if unit < 1024:
	if unit > 999:
	template = "%.4g %s"
	return template % (unit, suffix)

[slook]

I suppose actually you are correct that 'B' is the first item in the FILE_SIZE_SUFFIXES list, so there is not really any looping going on here, and no further optimization seems to be required anyway since the main performance issue is fixed as discussed below in #1881 (comment)

[slook]

FIXED in 3.2.x

If the speed is equal to 0 then it doesn't need updating more than one time. Perhaps some flag is needed that indicates the label string was already set to "0 B/s" once already.

That's what my commit did. Could you put in some logging to verify?

trayicon.py:

    def set_download_status(self, status):

        if self.menu is None:
            return

        log.add(status)
        self.downloads_item.set_label(status)

log output:

2022-07-20 11:50:24 Downloads: 0 B/s
2022-07-20 11:50:24 Connecting to server.slsknet.org:2242
2022-07-20 11:50:24 Downloads: 0 B/s
2022-07-20 11:50:24 Connected to server server.slsknet.org:2242, logging in…
2022-07-20 11:50:25 Rescan complete: 304 folders found
2022-07-20 11:50:25 1773 privileged users
2022-07-20 11:50:25 You have no privileges. Privileges are not required, but allow your downloads to be queued ahead of non-privileged users.
2022-07-20 11:50:25 Downloads: 0 B/s
2022-07-20 11:50:26 Downloads: 0 B/s
2022-07-20 11:50:27 Downloads: 0 B/s
2022-07-20 11:50:28 Downloads: 0 B/s
2022-07-20 11:50:29 Downloads: 0 B/s
2022-07-20 11:50:30 Downloads: 0 B/s
2022-07-20 11:50:31 Downloads: 0 B/s
2022-07-20 11:50:32 Downloads: 0 B/s
2022-07-20 11:50:33 Downloads: 0 B/s
2022-07-20 11:50:34 Downloads: 0 B/s
2022-07-20 11:50:35 Downloads: 0 B/s
2022-07-20 11:50:36 Downloads: 0 B/s
2022-07-20 11:50:37 Downloads: 0 B/s
2022-07-20 11:50:38 Downloads: 0 B/s
2022-07-20 11:50:39 Downloads: 0 B/s
2022-07-20 11:50:40 Downloads: 0 B/s
2022-07-20 11:50:41 Downloads: 0 B/s
2022-07-20 11:50:42 Downloads: 0 B/s
2022-07-20 11:50:43 Downloads: 0 B/s
2022-07-20 11:50:44 Downloads: 0 B/s
2022-07-20 11:50:45 Downloads: 0 B/s
2022-07-20 11:50:46 Downloads: 0 B/s
2022-07-20 11:50:47 Downloads: 0 B/s
2022-07-20 11:50:48 Downloads: 0 B/s
2022-07-20 11:50:49 Downloads: 0 B/s
2022-07-20 11:50:50 Downloads: 0 B/s
2022-07-20 11:50:51 Downloads: 0 B/s
2022-07-20 11:50:52 Downloads: 0 B/s
2022-07-20 11:50:53 Downloads: 0 B/s
2022-07-20 11:50:54 Downloads: 0 B/s
2022-07-20 11:50:55 Downloads: 0 B/s
2022-07-20 11:50:56 Downloads: 0 B/s
2022-07-20 11:50:57 Downloads: 0 B/s
2022-07-20 11:50:58 Downloads: 0 B/s
2022-07-20 11:50:59 Downloads: 0 B/s
2022-07-20 11:51:00 Downloads: 0 B/s
2022-07-20 11:51:01 Downloads: 0 B/s
2022-07-20 11:51:02 Downloads: 0 B/s
2022-07-20 11:51:03 Downloads: 0 B/s
2022-07-20 11:51:04 Downloads: 0 B/s
2022-07-20 11:51:05 Downloads: 0 B/s
2022-07-20 11:51:06 Downloads: 0 B/s
2022-07-20 11:51:07 Downloads: 0 B/s
2022-07-20 11:51:08 Downloads: 0 B/s
2022-07-20 11:51:09 Downloads: 0 B/s
2022-07-20 11:51:10 Downloads: 0 B/s
2022-07-20 11:51:11 Downloads: 0 B/s
2022-07-20 11:51:12 Downloads: 0 B/s
2022-07-20 11:51:13 Downloads: 0 B/s
2022-07-20 11:51:14 Downloads: 0 B/s
2022-07-20 11:51:15 Downloads: 0 B/s
2022-07-20 11:51:16 Downloads: 0 B/s
2022-07-20 11:51:17 Downloads: 0 B/s
2022-07-20 11:51:18 Downloads: 0 B/s

download_status_label.get_text() will always be different from download_bandwidth because the label is a string containing text whereas the bandwidth is an integer, so this condition doesn't work:

nicotine-plus/pynicotine/gtkgui/frame.py

Line 1815 in 716e997

if self.download_status_label.get_text() != download_bandwidth:

[mathiascode]

Sigh, brain fart moment...

[mathiascode]

Try the latest commits.

[slook]

The performance issue is solved at idle and "Downloads: 0 B/s" is correctly shown in the tray icon menu, including working correct during and after transfers are finished.

[slook]

FIXED regression in master Cannot download from self

The item is added into Downloads but no token exchange is attempted:

2022-07-20 13:17:40 [Transfer] Adding file lbry\filename.mp3 from user l_ to download queue

This ability serves no useful purpose other than for lazy testing, just an observation that the behaviour has changed since recently.

[mathiascode]

Regression somewhere in e60e29c. You can't read your own user info either.

[slook]

Confirmed here user info correctly shows Interests and Dislikes but no Self Description nor upload slot data shows only "Unknown":

[mathiascode]

Fixed in 7abd59e

[slook]

FIXED regression in Nicotine+ 3.2.3dev and 3.3.0dev - Critical Error on Browse Shares > Folder > File Properties > Previous file > Crash

Nicotine+ has encountered a critical error and needs to exit. Please copy the following message and include it in a bug report:

Nicotine+ Version: 3.3.0.dev1
GTK Version: 3.22.11
Python Version: 3.5.3 (default, Nov  4 2021, 15:29:10) [GCC 6.3.0 20170516]

Type: <class 'AttributeError'>
Value: 'FileProperties' object has no attribute 'current_index'
Traceback:   File "/home/user/Git/nicotine-plus/master/pynicotine/gtkgui/dialogs/fileproperties.py", line 82, in on_previous
    self.current_index -= 1

The crash also happens in the 3.2.x branch. It might seem to be triggered if the application was deactivated and then focused again, not always is the counter at the beginning, it can happen during the mid-way selection paging:

Edit: The crash might seem to only happens when switching to another application with Ctrl+Tab key, and then returning to it by clicking in the taskbar with the mouse.

• Open the File Properties dialog and begin to page backwards through the files to get mid-way at any index,
• Deactivate the main application (whilst the File Properties dialog is still open) by switching to a non-maximized window (that partially covers N+), and then
• click on the File Properties dialog which will then come to the foreground (but whilst the N+ main window is still partially behind the other previously active app window). Then,
• on the second time of clicking the 'Previous file' button (the first time is successful)
• the crash occurs after paging back for a second time.

[slook]

Perhaps instead of show() do run() the dialog instead?

Or what about storing the index in the same way as an immutable data parameter like total_size?

[mathiascode]

run() is no longer a thing in GTK 4.

0b3bbcf should fix the issue, but I need to look into message dialogs as well. I believe they suffer from the same problem.

[mathiascode]

a257260 fixes the issue for message dialogs as well, which should solve all the recent crashes you've reported.

[mathiascode]

Hm, doesn't seem to be enough for message dialogs yet. Looking into it.

[mathiascode]

Fixed for real in 360d542

[slook]

Feature suggest: Full-row select for lines in TextView

Suggest display left-handed arrow mouse-cursor in the margin area (and if possible also in the white-space between the user tag and the chat line, because it is not usual that the timestamp is desired for the selection).

Then on click release it would be ideal to select complete rows, or just the chat line to make it easier to copy and quote in messages, depending upon where the mouse cursor is positioned:

v                   v          v
 YYYY-MM-DD HH:MM:SS [username] line
^                   ^          ^

Idea: Clicking on any of the above ^ indicated zones would select all text from that column to the end of the row.

Additionally, if the cursor is positioned in whitespace to the right hand side, then full-row select would be desirable on click release with display of the standard arrow mouse cursor (i.e. the text caret cursor is only shown when it is over actual text), but without impeding the normal drag select functionality.

[slook]

Feature suggest: Quote selection in Chat TextView

A "Quote" menu item and/or shortcut that simply inserts the currently selected text string into the Chat TextEntry without copying it into the clipboard, and then grabs the focus to the TextEntry to allow the user to quickly comment on the inserted quote.

This could be a good middle-click function to implement (perhaps even if no selection exists, then the word that is under the cursor is inserted instead).

Additionally, a middle-click gesture shortcut on the Username tag might be an idea for a quick way to mention a user in a chat message.

[slook]

DONE in Nicotine+ 3.3.0 - Feature request: Expand into first level upon initial loading of Browse Shares list, instead of fully expanding all

[_3] Could you guys add a feature to auto collapse shares instead of always getting the exploded view? Its annoying to have to go through a share every time and close all the big folders.

More than a few users find the initial expanded view of the shares list to be unhelpful. We've looked into this before and TreeView does not have any built-in method of doing this.

It would be desirable to have only the first level nodes below the roots expanded upon loading the initial view, because the fully collapsed view is normally useless because it shows only the root nodes which is not enough to start browsing quickly.

[mathiascode]

I'm not yet certain if remembering based on the last button state is desirable or not, since it is changed per user tab which might create confusing behaviour if that button is frequently pressed without the users knowledge that this affects the future behaviour for the next new browse operation.

I'm not going to add more options here or spend much time on this request right now, Take it or leave it.

[slook]

The experimental feature causes inconsistant behaviour so it needs more work, imo should be left out of the 3.2.3 branch for the time being.

Or the line which sets the config setting in userbrowse.py can be left out such to enable manual configuration only.

[mathiascode]

fine, i'll revert it then. someone else will have to put in the work, i'm too tired for that

[slook]

Thank you. For now this is not an urgent issue, I will be sure to look at it in the near future.

[slook]

Proposed implementation in #2434

[slook]

FIXED in Nicotine+ 3.2.4 - Buffer Overflow Critical Error when downloading a massive file whilst another massive download is paused

Some initial findings from the testers doing experimental bug finding...

[_3] My friend got a buffer overflow error
[l_] on what system?
[_3] Windows 10
[l_] it sounds like a windows error, I am not aware of any buffer overflow issues in nicotine
[_3] He was downloading a 95GB file from me, paused it, then started another 8gb download from me.
[l_] such a crash can only be investigated if it can be reproduced, perhaps your friend can try to make it happen again
[_3] I think it happened because he filled his 8gb of ram when he paused a download and then overflowed it by starting another.
[l_] oh interesting, yes perhaps indeed there is some memory management issue. it would sure be good to investigate it further.
[_3] Yeah he said n+ told him to restart the program but that may not be true, he isnt very tech savvy
[l_] Next time it happens tell him to copy the text from the Critical Error dialog box or take a screenshot of it
[_3] Il let him know.
[l_] cool, if you can capture an error message or traceback, then I will create an issue on the tracker
[_3] Ive started adding some very large files on here so there may be some things I run into as I know this program isnt really designed for that
[l_] yes testing and debugging the performance of massive file transfers is going to be worked on for 3.3.0+, we appreciate your help on that.

[_3] Nicotine+ has encountered a critical error and needs to exit. Please copy the following message and include it in a bug report:
[_3] Nicotine+ Version: 3.2.2
[_3] GTK Version: 3.24.33
[_3] Python Version: 3.9.10 (main, Mar 14 2022, 18:37:07)  [GCC 11.2.0 64 bit (AMD64)]
[_3] Type: <class 'OverflowError'>
[_3] Value: 10493118292 not in range -2147483648 to 2147483647
[_3] Traceback:   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/pynicotine.py", line 464, in network_event
[_3]   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/pynicotine.py", line 507, in file_download
[_3]   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/transfers.py", line 1302, in file_download
[_3]   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/transfers.py", line 1967, in update_download
[_3]   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/gtkgui/transferlist.py", line 303, in update
[_3]   File "D:/a/nicotine-plus/nicotine-plus/pynicotine/gtkgui/transferlist.py", line 479, in update_specific
[_3]   File "D:/a/_temp/msys64/mingw64/lib/python3.9/site-packages/gi/overrides/Gtk.py", line 1309, in set_value
[_3]   File "D:/a/_temp/msys64/mingw64/lib/python3.9/site-packages/gi/overrides/Gtk.py", line 921, in _convert_value
[_3]   File "D:/a/_temp/msys64/mingw64/lib/python3.9/site-packages/gi/overrides/GObject.py", line 208, in init
[_3] "D:/a/_temp/msys64/mingw64/lib/python3.9/site-packages/gi/overrides/GObject.py", line 257, in set_value

Note that during normal operation there is not any issue. Only the very massive download.

the 10493118292 seconds estimated time remaining is 332 years, but only 68 years is allowable with an int

nicotine-plus/pynicotine/gtkgui/transferlist.py

Line 127 in cdab528

int, # (15) time left

Perhaps if the estimated Time Left is more than a certain value, we should cap it...

nicotine-plus/pynicotine/gtkgui/transferlist.py

Line 461 in cdab528

left = transfer.time_left or 0

nicotine-plus/pynicotine/gtkgui/transferlist.py

Line 479 in cdab528

self.transfersmodel.set_value(initer, 15, left)

[mathiascode]

Could you create a new comment if you make substantial edits? I don't get notifications for edits.

[mathiascode]

Fixed in cc5e680

[slook]

FIXED in Nicotine+ 3.2.4 - No folder on a certain USB attached NTFS drive cannot be added into Shares using Preferences dialog

Latest master 3.3.0dev1

User reports the folder can be browsed and given a Virtual name which is added into the Shares dialog page, but is disappeared after the Preferences dialog is closed and re-opened. Meanwhile the shared setting in config is never updated.

All other drives on the same system are okay and can be added without problem. The affected drive is an external Basic volume device identical to another drive in the same USB caddy attached to the system which works fine, i.e. folders on the other drive can be added fine. The two drives each have their own drive letter in Windows but appear together in the list of devices that can be safely ejected.

The problem drive can be added using Setup Assistant, but not the Prefrences dialog.

---

see 9e1796f

[slook]

This thread is closed.

Post new topics in the new thread:
[3.3.0.dev2] #2145