Network Remote V3 #1747

poldi171254 · 2025-05-18T06:22:26Z

I have now made the Network Remote optional and cleaned up the code as per request.
This replaces V2 pull request.
I tried to close my previous pull request, but can't find a way of doinng this

poldi171254 · 2025-05-27T07:32:38Z

I'd appreciate of you could find the time for a review so I can maybe focus on working on some other areas of Strawberry.
Also, as I mentioned before, I cannot close my previous pull request Network Remote V2.

jonaski · 2025-05-27T20:50:08Z

I'd appreciate of you could find the time for a review so I can maybe focus on working on some other areas of Strawberry. Also, as I mentioned before, I cannot close my previous pull request Network Remote V2.

I already reviewed in the previous PR #1688, most of the issues are not addressed.

jonaski

Most of the issues are not addressed, there is also still lot's of formatting issues, it's not consistent.
I also made some additional comments.

src/networkremote/remotesettings.cpp

src/networkremote/tcpserver.h

src/networkremote/client.cpp

src/networkremote/client.h

jonaski · 2025-05-27T20:51:07Z

src/settings/backendsettingspage.ui

@@ -132,7 +132,7 @@
         <item>
          <spacer name="spacer_alsaplugin">
           <property name="orientation">
-            <enum>Qt::Horizontal</enum>
+            <enum>Qt::Orientation::Horizontal</enum>


These changes shouldn't be part of the PR since it's unrelated to the network remote.

Agree, but not sure how it got there as I did not make any changes or how to resolve this

You pulled this in with a merge, this is unnecessary noise in a PR, don't mark it resolved when it clearly isn't

jonaski · 2025-05-27T20:54:50Z

I don't understand why your merging in a bunch of unrelated commits here, there is no commit here explaining what's being done here.

jonaski · 2025-05-27T20:57:30Z

Commit 8e158ad looks strange, it's a mix my changes and your changes, the text in the commit title is from a different commit, while the commit message body is a mix of different comments.

poldi171254 · 2025-05-28T01:25:31Z

I think one of the recurring issues seems to be the format of the code.

I'm using Qt Creator

If you are using CLangFormat settings maybe you can share your .clang_format file

If you you are using something else for your development, maybe you can share what that is

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 I have fixed most of the code issues apart from the one I commented on 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 Changes requested by maintainer Added Copyright Passing Player instead of Application as an argument Clean code as per maintainers request Using Player rather than Application class to get song data Code cleanup Removed RPATH related entries form cmake file Fixed code as per maintainers reuqest Removed all non essential refererences to a Application from the code Inserted Copyright Stage 1 of making Network Remote optional

poldi171254 · 2025-06-03T03:29:22Z

Commit 8e158ad looks strange, it's a mix my changes and your changes, the text in the commit title is from a different commit, while the commit message body is a mix of different comments.

I think I made mistakes when I squashed the commits. Any idea how to fix this?

poldi171254 · 2025-06-05T08:17:22Z

I've gone through your requests for V2 PR and as far as I can see I've done them all.
I'd like to move forward and suggest that we delete the V2 PR and focus on V3.

jonaski · 2025-06-05T18:04:19Z

I think one of the recurring issues seems to be the format of the code.

I'm using Qt Creator

If you are using CLangFormat settings maybe you can share your .clang_format file

There already is a clang-format file in this repository as I explained here:
#1688 (review)

If you you are using something else for your development, maybe you can share what that is

jonaski · 2025-06-05T18:09:19Z

Commit 8e158ad looks strange, it's a mix my changes and your changes, the text in the commit title is from a different commit, while the commit message body is a mix of different comments.

I think I made mistakes when I squashed the commits. Any idea how to fix this?

I'm not sure you can fix it with a normal rebase since you have changed commits that were part of master.
I think you need to undo all the commits without deleting the the changes, then commit again and force push:
Do git reset HEAD^ until you reach a clean commit, then commit your changes and rebase with master to get back all the deleted commits.
I suggest to make a copy of the branch in case you accidentally delete something.

jonaski · 2025-06-05T17:35:58Z

CMakeLists.txt

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


As already explained before, this should be combined with existing find_package.

CMakeLists.txt

jonaski · 2025-06-05T17:36:57Z

CMakeLists.txt

-target_link_libraries(strawberry PUBLIC strawberry_lib)
+if(HAVE_NETWORKREMOTE)
+  add_subdirectory(src/networkremote)
+  target_link_libraries(strawberry_lib PRIVATE Qt6::Protobuf)


Combine with the existing target_link_libraries

src/core/application.cpp

src/core/application.h

src/core/application.cpp

jonaski · 2025-06-05T18:01:07Z

src/networkremote/clientmanager.h

+
+ private:
+  const SharedPtr<Player> player_;
+  QList<NetworkRemoteClient*> clients_;


Can QSharedPointer be used for clients?

jonaski · 2025-06-05T18:01:31Z

src/networkremote/incomingmsg.cpp

+
+NetworkRemoteIncomingMsg::NetworkRemoteIncomingMsg(QObject *parent)
+    : QObject(parent),
+      msg_(new nw::remote::Message),


Why is this needed?

jonaski · 2025-06-05T18:02:05Z

src/networkremote/incomingmsg.cpp

+}
+
+void NetworkRemoteIncomingMsg::SetMsgType() {
+  msg_string_ = msg_stream_.toStdString();


Use Qt Protobuf

src/networkremote/incomingmsg.h

src/core/application.cpp

jonaski · 2025-06-05T18:17:04Z

src/networkremote/clientmanager.cpp

+void NetworkRemoteClientManager::StateChanged() {
+  QTcpSocket *socket = qobject_cast<QTcpSocket*>(sender());
+  if (!socket) return;
+  qLog(Debug) << socket->state();


These log messages need to be improved.

Can you be more specific?

src/networkremote/clientmanager.h

jonaski · 2025-06-05T18:18:11Z

src/networkremote/clientmanager.h

@@ -0,0 +1,47 @@
+/*


Incorrect filename

What's wrong with it?

jonaski · 2025-06-05T18:19:20Z

src/networkremote/networkremote.cpp

+#include "core/application.h"
+#include "core/logging.h"
+
+NetworkRemote* NetworkRemote::sInstance_ = nullptr;


Remove singleton

src/networkremote/networkremote.cpp

jonaski · 2025-06-05T18:20:35Z

src/networkremote/networkremote.h

+  bool enabled_;
+  bool local_only_;
+  int remote_port_;
+  QHostAddress ipAddr_;


Use snake case for variable names

jonaski · 2025-06-05T18:20:42Z

src/networkremote/networkremote.h

+  int remote_port_;
+  QHostAddress ipAddr_;
+  NetworkRemoteTcpServer *server_;
+  static NetworkRemote *sInstance_;


jonaski · 2025-06-05T18:24:05Z

src/networkremote/remotesettings.cpp

+  for (const QHostAddress &address : std::as_const(hostList)) {
+    if (address.protocol() == QAbstractSocket::IPv4Protocol && address.isLoopback() == false && !found) {
+    qInfo( "Warning: The code only picks the first IPv4 address");
+      found = true;


Wrong indent

jonaski · 2025-06-05T18:24:49Z

src/networkremote/remotesettings.cpp

+void NetworkRemoteSettings::SetIpAdress() {
+  bool found = false;
+  const QList<QHostAddress> hostList = QNetworkInterface::allAddresses();
+  for (const QHostAddress &address : std::as_const(hostList)) {


You don't need std::as_const since you already declare hostList const

jonaski · 2025-06-05T18:25:06Z

src/networkremote/remotesettings.cpp

+
+void NetworkRemoteSettings::SetIpAdress() {
+  bool found = false;
+  const QList<QHostAddress> hostList = QNetworkInterface::allAddresses();


Use snake case for variable names

jonaski · 2025-06-05T18:25:39Z

src/networkremote/remotesettings.h

+  void SetPort(int);
+
+ private:
+  Settings s_;


No need to keep s_ a class member, declare setting where you use it.

jonaski · 2025-06-05T18:26:34Z

src/networkremote/tcpserver.h

@@ -0,0 +1,46 @@
+/*


Incorrect filename, filename should match lowercase class name

src/core/application.cpp

…le changes This should all be ok now

You need to restart Strawberry for this setting to take affect

I now have a working version with option network remote code. I still need to check the maintainers latest comments.

…into NWRemoteOptional

jonaski requested changes May 27, 2025

View reviewed changes

jonaski and others added 5 commits May 30, 2025 11:16

BackendSettingsPage: Add tooltip for HTTP/2

0a4fd6e

You need to restart Strawberry for this setting to take affect

The Network Remote is now an optional component

e8a672c

Fixed a bug when strawberry.conf did not exist

f9b5b00

Code cleanup

6c51a2c

poldi171254 force-pushed the NWRemoteOptional branch from af9306e to f9b5b00 Compare May 30, 2025 01:22

lister54 added 2 commits May 30, 2025 12:17

removed incorrect pointer references

28e3dff

Mafe QList const

1e1dd2a

jonaski requested changes Jun 5, 2025

View reviewed changes

jonaski reviewed Jun 5, 2025

View reviewed changes

src/core/application.cpp Show resolved Hide resolved

jonaski reviewed Jun 5, 2025

View reviewed changes

src/networkremote/clientmanager.h Show resolved Hide resolved

jonaski requested changes Jun 5, 2025

View reviewed changes

jonaski reviewed Jun 5, 2025

View reviewed changes

src/core/application.cpp Show resolved Hide resolved

jonaski reviewed Jun 5, 2025

View reviewed changes

src/core/application.cpp Show resolved Hide resolved

poldi171254 and others added 5 commits June 10, 2025 09:56

I think I made mistakes when squasing commits and lost some of the fi…

94212e3

…le changes This should all be ok now

BackendSettingsPage: Add tooltip for HTTP/2

d64a93f

You need to restart Strawberry for this setting to take affect

I think some of the squashes broke my code

8605c31

I now have a working version with option network remote code. I still need to check the maintainers latest comments.

The Network Remote is now an optional component

94c14f9

Fixed a bug when strawberry.conf did not exist

d362879

lister54 and others added 4 commits June 10, 2025 18:29

removed incorrect pointer references

cd8fd75

Merge branch 'NWRemoteOptional' of github.com:poldi171254/strawberry …

3c5af2b

…into NWRemoteOptional

Formatted code

b317f2c

Networkremote now uses Qt6 Protobuf implementation

a051e40

Uh oh!

Network Remote V3 #1747

Are you sure you want to change the base?

Network Remote V3 #1747

Uh oh!

Conversation

poldi171254 commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poldi171254 commented May 27, 2025

Uh oh!

jonaski commented May 27, 2025

Uh oh!

jonaski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poldi171254 May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonaski commented May 27, 2025

Uh oh!

jonaski commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poldi171254 commented May 28, 2025

Uh oh!

poldi171254 commented Jun 3, 2025

Uh oh!

poldi171254 commented Jun 5, 2025

Uh oh!

jonaski commented Jun 5, 2025

Uh oh!

jonaski commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

poldi171254 commented May 18, 2025 •

edited

Loading

poldi171254 May 30, 2025 •

edited

Loading

jonaski commented May 27, 2025 •

edited

Loading