Network Remote V2 #1688

lister54 · 2025-03-01T06:13:59Z

This is an updated version of the code I wrote originally. As I wanted to keep track of changes in the original I creates a new fork and appliedf my changes there.

I also have workign clients for Linux and Android and am hoping somebody else is interested in this feature,

I have one outstanding issue on Ubuntu
[1276/1276 11.5/sec] Linking CXX executable strawberry
/usr/bin/ld: warning: libicui18n.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicui18n.so.74
/usr/bin/ld: warning: libicuuc.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicuuc.so.74

but the app is working fine otherwise.
I've built and run the app in Qt Creator 15.0.1

… solution

poldi171254 · 2025-03-04T22:43:08Z

Do I need to do anything else to get this approved?

jonaski

I'm stopping review now, too many issues here:

Use rebase instead of merge with master so the new changes gets added on top of master.
Squash any commits that are corrections to the previous commits.
Remove all changes which are not related to the new feature, like newlines elsewhere, or unrelated changes.
The formatting of the code is inconsistent with the codebase, please use 2 spaces for indent everywhere, curly brace on the same line as the function declaration, snake case for variable names everywhere. Consider running it through clang-format to fix some of it (set DisableFormat to false in .clang-format), but only run it on the new code.
The naming is too generic, ie.: "Client", either everything should be namespaced inside NetworkRemote, or prefix all classes with NetworkRemote. I prefer the latter since that's the style we use.
Use Qt stringliterals (https://doc.qt.io/qt-6/qt-literals-stringliterals.html) instead of QStringLiteral / QLatin1String (Except for in headers).
Use Qt Protobuf (https://doc.qt.io/qt-6/qtprotobuf-index.html) instead of protobuf directly, that avoids all the string conversions and use of std::string.
Remove Application and include only the objects you use.
Use Settings instead of QSettings (core/settings.h), this is so we can add some modifications to where the config is stored.
Some objects are allocated on heap and not free'd that could be allocated on the stack.
I think we should use signals/slots to reload settings (like most other places) instead of the singleton.
Add missing copyright headers.
Make network remote a optional feature with optional_component

I started porting the networkremote code from Clementine to Qt 6 and Qt Protobuf a while ago, but the code in Clementine had a lot of issues. I've published it in the networkremote branch, it's unfinished and basically a mess, so it requires a lot more work, but it has more features than you've added here, but I'm not sure when I have time to finish it.

CMakeLists.txt

data/icons.qrc

src/networkremote/networkremote.h

jonaski · 2025-03-05T20:10:59Z

src/networkremote/networkremote.h

+  int remote_port_;
+  QHostAddress ipAddr_;
+  TcpServer *server_;
+  QThread *original_thread_;


remote_port_ is used in networkremote.cpp

original_thread_ is not used, that's the line I commented on

Its still here

src/networkremote/networkremote.h

src/networkremote/networkremote.cpp

poldi171254 · 2025-03-05T22:35:41Z

Happy to work on all the changes you requested if that get's the functionality over the line; I may need some help/clarification on some of the issues though
I appreciate that the Clementine Remote had more functions, but when I started I thought that some basic functionality was better than not having this at all

Fixes strawberrymusicplayer#1680

You need to restart Strawberry for this setting to take affect

You need to restart Strawberry for this setting to take affect.

Renamed classes to be less ambiguous Save before clientmanager changes This works after major change in clientmanager Cleaned up formatting Still works

…n where I wasn't sure what was meant or how to resolve the issue. I reformatted all my code and used Qt stringliterals where applicable Cmake now uses Qt Protobuf I removed Application where I could. Still needed where I need acces to the player. All objects should now be freed. I have hardcoded the CMAKE_INSTALL_RPATH as RPATH seems to be a requirement when using Ninja as the cmake generator. Ubuntu currently comes with 6.4.2, but the QtCreator is on 6.8 I'm pulling in the library from the Creator Not sure what you meant by "I think we should use signals/slots to reload settings (like most other places) instead of the singleton" Not sure what the Copyright header should look like. Do I put my name on this? I have not made the Network remote optional yet as I wanted to get the code sorted out first I still have have one outstanding issue on Ubuntu [1276/1276 11.5/sec] Linking CXX executable strawberry /usr/bin/ld: warning: libicui18n.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicui18n.so.74 /usr/bin/ld: warning: libicuuc.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicuuc.so.74

poldi171254 · 2025-04-23T03:54:28Z

I have resolved most of the issues you have raised, but have left some of them unresolved as they have my comments and need in some cases answers/clarification

Is there a standard copyright I canshould use?

I will make the Network remote optional in the meantime

jonaski · 2025-04-23T17:21:25Z

CMakeLists.txt

@@ -29,6 +29,10 @@ endif()
 if(LINUX)
  include(cmake/Rpm.cmake)
  include(cmake/Deb.cmake)
+  # RPATH seems to be a requirement when using Ninja as the cmake generator.
+  # as Ubuntu currently comes with 6.4.2, but the QtCreator is on 6.8 I'm pulling in the library from the Creator
+  set(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE)


I don't know why you need this, but this shouldn't be hard-coded in CMakeLists.txt, you can easily pass -DCMAKE_BUILD_WITH_INSTALL_RPATH=TRUE to cmake if you need this.

jonaski · 2025-04-23T17:23:30Z

CMakeLists.txt

+  # RPATH seems to be a requirement when using Ninja as the cmake generator.
+  # as Ubuntu currently comes with 6.4.2, but the QtCreator is on 6.8 I'm pulling in the library from the Creator
+  set(CMAKE_BUILD_WITH_INSTALL_RPATH TRUE)
+  set(CMAKE_INSTALL_RPATH "/home/llist/Qt/6.9.0/gcc_64/lib")


Same with this

jonaski · 2025-04-23T20:32:51Z

I have resolved most of the issues you have raised, but have left some of them unresolved as they have my comments and need in some cases answers/clarification

The formatting is still wrong, I don't know if you have pushed all the changes, I see mostly unrelated commits merged in. Please rebase with master to clean it up.

Is there a standard copyright I canshould use?

You can just copy one from the other files.

jonaski · 2025-04-23T20:35:47Z

This is an updated version of the code I wrote originally. As I wanted to keep track of changes in the original I creates a new fork and appliedf my changes there.

I also have workign clients for Linux and Android and am hoping somebody else is interested in this feature,

I have one outstanding issue on Ubuntu [1276/1276 11.5/sec] Linking CXX executable strawberry /usr/bin/ld: warning: libicui18n.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicui18n.so.74 /usr/bin/ld: warning: libicuuc.so.73, needed by /home/llist/Qt/6.8.2/gcc_64/lib/libQt6Core.so.6.8.2, may conflict with libicuuc.so.74

but the app is working fine otherwise. I've built and run the app in Qt Creator 15.0.1

It seems like you have 2 conflicting ICU libraries, one on your system and one in /home/llist/Qt/6.8.2/gcc_64/lib/, strawberry probably links your systems version so you need to pass flags to cmake to make it use the same version used by the Qt installation, otherwise you'll have 2 conflicting versions linked which can lead to collision. This is unrelated to the changes so this should be removed from the PR description and commit message.

poldi171254 · 2025-05-01T07:44:17Z

I think I have now completed the code as per your feedback.
I'd appreciate if you could review the code before I attempt to resolve the cmake issues.

jonaski

I suggest to run the code through formatting first to fix all the incorrect indent and variable names

jonaski · 2025-05-01T21:07:16Z

CMakeLists.txt

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.13)
+cmake_minimum_required(VERSION 3.14)


Why is minimum CMake version increased?

No particular need, but thought it didn't hurt being on a later version given that I didn't find any problems.
I have changed this back to 3.13

jonaski · 2025-05-01T21:12:40Z

CMakeLists.txt

@@ -172,6 +176,15 @@ if(UNIX AND NOT APPLE)
  endif()
  find_package(X11 COMPONENTS X11_xcb)
 endif()
+
+find_package(Qt6 REQUIRED COMPONENTS Protobuf)


Using multiple find_package lines for the same library is bad practice. Move Protobuf to QT_OPTIONAL_COMPONENTS

I'm really out of my depth here and used a solution I found from examples & tutorials.
I need the first find_package for the Qt6 protobuf package and the second find_package for protobuf_generate.
If I comment any of them out cmake fails.

jonaski · 2025-05-01T21:13:10Z

CMakeLists.txt

+
+find_package(Qt6 REQUIRED COMPONENTS Protobuf)
+if(NOT Protobuf_FOUND)
+  find_package(Protobuf REQUIRED)


Remove: No need to use Protobuf directly.

jonaski · 2025-05-01T21:13:13Z

CMakeLists.txt

@@ -172,6 +176,15 @@ if(UNIX AND NOT APPLE)
  endif()
  find_package(X11 COMPONENTS X11_xcb)
 endif()
+
+find_package(Qt6 REQUIRED COMPONENTS Protobuf)
+if(NOT Protobuf_FOUND)


Remove: No need to use Protobuf directly.

jonaski · 2025-05-01T21:13:17Z

CMakeLists.txt

+find_package(Qt6 REQUIRED COMPONENTS Protobuf)
+if(NOT Protobuf_FOUND)
+  find_package(Protobuf REQUIRED)
+endif()


Remove: No need to use Protobuf directly.

jonaski · 2025-05-01T21:33:42Z

src/networkremote/tcpserver.h

+
+class NetworkRemoteTcpServer : public QObject
+{
+  Q_OBJECT


1 space for indent

Not sure what you mean

jonaski · 2025-05-01T21:33:46Z

src/networkremote/tcpserver.h

+  void StartServer(QHostAddress ipAddr, int port);
+  void StopServer();
+
+private:


1 space for indent

jonaski · 2025-05-01T21:33:57Z

src/networkremote/tcpserver.h

+  Application *app_;
+  QTcpServer *server_;
+  QTcpSocket *socket_;
+  NetworkRemoteClientManager *clientMgr_;


Use snake case for variables

jonaski · 2025-05-01T21:36:43Z

src/networkremote/outgoingmsg.cpp

+  : QObject(parent),
+    app_(app),
+    msg_(new nw::remote::Message),
+    responeSong_(new nw::remote::ResponseSongMetadata)


jonaski · 2025-05-01T21:37:04Z

src/networkremote/outgoingmsg.cpp

+
+  bytesOut_ = msg_->ByteSizeLong();
+
+  if(socket_->isWritable())


Suggested change

if(socket_->isWritable())

if (socket_->isWritable())

jonaski · 2025-05-01T21:47:54Z

Commit 18de736 looks wrong, it looks like you modified my commit with changes from this PR.
Like I pointed out earlier please rebase with master to avoid the commit mess.

jonaski · 2025-05-01T22:14:40Z

src/settings/networkremotesettingspage.cpp

+{
+  ui_->setupUi(this);
+  const int iconSize = style()->pixelMetric(QStyle::PM_TabBarIconSize);
+  setWindowIcon(IconLoader::Load(QStringLiteral("network-remote"), true, 0,iconSize));


Suggested change

setWindowIcon(IconLoader::Load(QStringLiteral("network-remote"), true, 0,iconSize));

setWindowIcon(IconLoader::Load(u"network-remote"_s), true, 0, style()->pixelMetric(QStyle::PM_TabBarIconSize)));

Gives the following warning in Qt Creator
networkremotesettingspage.cpp:39:56: Too many arguments to function call, expected single argument 'icon', have 4 arguments
qwidget.h:378:10: 'setWindowIcon' declared here

jonaski · 2025-05-01T22:15:11Z

src/settings/networkremotesettingspage.cpp

+void NetworkRemoteSettingsPage::Load()
+{
+  ui_->portSelected->setRange(5050, 65535);
+  ui_->ip_address->setText(QStringLiteral("0.0.0.0"));


You included Qt::Literals::StringLiterals but you forgot to actually use it.

jonaski · 2025-05-01T22:15:27Z

src/settings/networkremotesettingspage.cpp

+
+  ui_->useRemoteClient->setCheckable(true);
+  ui_->useRemoteClient->setChecked(settings_->UserRemote());
+  if (settings_->UserRemote()){


Suggested change

if (settings_->UserRemote()){

if (settings_->UserRemote()) {

poldi171254 and others added 5 commits February 17, 2025 17:14

fixed cmake RPATH issue by ignoring the path; this is not the correct…

df2219d

… solution

Just testing push

c5cc6f8

Commit due to moving to another dev env

4680f1b

Clean Build

9f665b1

Merge branch 'master' into master

8772903

jonaski requested changes Mar 5, 2025

View reviewed changes

jonaski mentioned this pull request Mar 5, 2025

Network Remote #1398

Closed

jonaski and others added 14 commits April 7, 2025 18:14

Fix issue template

6fe3726

nsi: Bump libFLAC version

61e15ae

FilesystemMusicStorage: Use QFile::supportsMoveToTrash

e19d02a

FilterParser: Update tooltip

f70f74b

Fixes strawberrymusicplayer#1680

CI: Fix Mageia build

47e0bc6

CI: Fix OpenMandriva build

7b45f0e

tests: Remove unused TestObjectDecorators

45fba4e

Equalizer: Add tooltip that playback must be restarted

948aa24

BackendSettingsPage: Add tooltip for HTTP/2

2902812

You need to restart Strawberry for this setting to take affect

AppearanceSettingsPage: Add tooltip about restart

d7b2222

You need to restart Strawberry for this setting to take affect.

nsi: Update sqlite3 dll name

18de736

Renamed classes to be less ambiguous Save before clientmanager changes This works after major change in clientmanager Cleaned up formatting Still works

Merge fixes into PR strawberrymusicplayer#1688

03908ca

Merge branch 'master' into NetworkRemoteV2

1e2b7b5

jonaski reviewed Apr 23, 2025

View reviewed changes

Merge branch 'master' into NetworkRemoteV2

3ab91ce

jonaski requested changes May 1, 2025

View reviewed changes

jonaski reviewed May 1, 2025

View reviewed changes

jonaski requested changes May 1, 2025

View reviewed changes

		@@ -1,4 +1,4 @@
		cmake_minimum_required(VERSION 3.13)
		cmake_minimum_required(VERSION 3.14)

	setWindowIcon(IconLoader::Load(QStringLiteral("network-remote"), true, 0,iconSize));
	setWindowIcon(IconLoader::Load(u"network-remote"_s), true, 0, style()->pixelMetric(QStyle::PM_TabBarIconSize)));

Network Remote V2 #1688

Are you sure you want to change the base?

Network Remote V2 #1688

Conversation

lister54 commented Mar 1, 2025

poldi171254 commented Mar 4, 2025

jonaski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poldi171254 commented Mar 5, 2025

poldi171254 commented Apr 23, 2025

jonaski Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonaski commented Apr 23, 2025

jonaski commented Apr 23, 2025 • edited Loading

poldi171254 commented May 1, 2025

jonaski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonaski commented May 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonaski left a comment •

edited

Loading

jonaski Apr 23, 2025 •

edited

Loading

jonaski commented Apr 23, 2025 •

edited

Loading