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

Expose stop tracker timeout in Advanced Settings (GUI + WebUI) #11834

Merged
merged 1 commit into from Jan 8, 2020
Merged

Expose stop tracker timeout in Advanced Settings (GUI + WebUI) #11834

merged 1 commit into from Jan 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 6, 2020

Exposing the settings for stop_tracker_timeout so user can set it to higher value to ensure tracker announces go through when client is exited/torrent is paused.

@ghost
Copy link
Author

ghost commented Jan 6, 2020

Dear @Chocobo1 @glassez please review it. Thanks!

Copy link
Member

@FranciscoPombal FranciscoPombal left a comment

Choose a reason for hiding this comment

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

In general:

  • You should try to make sure the new code for this setting appears at roughly in the same order relative to other settings in all relevant source files. This is not the case at the moment.

    For example, in src/base/bittorrent/session.cpp, you put the new code between m_asyncIOThreads... and m_filePoolSize... but in src/base/bittorrent/session.h you put it between RefreshInterval... and PreallocationEnabled....
    It is just a matter of fixing the order.

  • There are a couple of places where indentation is bad/missing.
    EDIT: it appears most of the indentation problems are due to the use to tab instead of spaces

  • There are a couple of places where you named variables/function names etc with snake_case when they should have been camelCase

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/base/bittorrent/session.h 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/views/preferences.html Outdated Show resolved Hide resolved
src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
@sledgehammer999
Copy link
Member

sledgehammer999 commented Jan 6, 2020

I am skeptical about the need of exposing this setting. It seems a bit unnecessary. Can't it be set to a sane default?

@FranciscoPombal
Copy link
Member

@sledgehammer999

I am skeptical about the need of exposing this setting. It seems a bit unnecessary. Can't it be set to a sane default?

There is already ongoing discussion about whether or not the default value for this setting should change, take a look: #11807

Regardless of the final decision on that matter, I think it is already clear that there are different enough use cases that warrant this setting be configurable. It is not a one-size-fits-all.

@ghost
Copy link
Author

ghost commented Jan 6, 2020

In general:

  • You should try to make sure the new code for this setting appears at roughly in the same order relative to other settings in all relevant source files. This is not the case at the moment.
    For example, in src/base/bittorrent/session.cpp, you put the new code between m_asyncIOThreads... and m_filePoolSize... but in src/base/bittorrent/session.h you put it between RefreshInterval... and PreallocationEnabled....
    It is just a matter of fixing the order.
  • There are a couple of places where indentation is bad/missing.
    EDIT: it appears most of the indentation problems are due to the use to tab instead of spaces
  • There are a couple of places where you named variables/function names etc with snake_case when they should have been camelCase

Thanks for your review. I tried to fix the mentioned issues as best as I could.

I am skeptical about the need of exposing this setting. It seems a bit unnecessary. Can't it be set to a sane default?

A sane default is necessay but this will allow user to tweak the settings to their use case.

@FranciscoPombal
Copy link
Member

Thanks for your review. I tried to fix the mentioned issues as best as I could.

No problem. Only a few minor things left. I will post the next review shortly.

Copy link
Member

@FranciscoPombal FranciscoPombal left a comment

Choose a reason for hiding this comment

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

You still need to move a couple of code snippets to the correct place to be 100% consistent.
Additionally, you forgot the libtorrent documentation help links in the settings (both webUI and gui).

src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved
src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
src/gui/advancedsettings.h Outdated Show resolved Hide resolved
src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
@FranciscoPombal
Copy link
Member

FranciscoPombal commented Jan 6, 2020

Looks good. Now you have to squash the commits before this can be merged.

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved
src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved
@ghost ghost requested a review from Chocobo1 January 7, 2020 09:42
Chocobo1
Chocobo1 previously approved these changes Jan 8, 2020
Copy link
Member

@glassez glassez left a comment

Choose a reason for hiding this comment

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

The period shouldn't appear at the end of commit message subject (so-called commit title).

@ghost
Copy link
Author

ghost commented Jan 8, 2020

The period shouldn't appear at the end of commit message subject (so-called commit title).

Fixed. Should the default value remain at 1 s?

@glassez
Copy link
Member

glassez commented Jan 8, 2020

Should the default value remain at 1 s?

I have no strong preferences about it (if it works in general case).
If it affects application shutdown time I wouldn't increase it.
Anyway I think changing default value is separate issue.

@Chocobo1 Chocobo1 merged commit 0578605 into qbittorrent:master Jan 8, 2020
@Chocobo1
Copy link
Member

Chocobo1 commented Jan 8, 2020

@An0n666
Thanks!

@ghost ghost deleted the an0n666-expose-stop-tracker-timeout branch January 8, 2020 10:11
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.

5 participants