@Kate6 thanks for the fixes. It doesn't look like any upnp devs are around for review. It might help if you complete the PR template, such as with more details about the fixes, why they're needed, and how they've been tested. Try to target a dev unfamiliar with upnp, because that's pretty much all who could review this.

@claude review this please

Claude finished @garbear's task in 1m 22s —— View job

Code Review Complete ✓

I've reviewed both UPnP fixes in this PR. Overall, these are well-targeted bug fixes that address real issues. Here's my detailed analysis:

Fix #1: Android UPnP Service Restart on Resume ✅

File: xbmc/application/Application.cpp:1551

Change:

case TMSG_RESUMEAPP:
{
  m_ServiceManager->GetNetwork().NetworkMessage(CNetworkBase::SERVICES_UP, 0);  // Added
  CGUIComponent* gui = CServiceBroker::GetGUI();
  ...

Analysis:

• Problem Addressed: When Android apps are backgrounded and resumed, network services (including UPnP) weren't being restarted
• Solution: Calls NetworkMessage(SERVICES_UP) which triggers m_services->Start() in Network.cpp:216
• Correctness: ✅ The fix mirrors the pattern used in ProfileManager.cpp:396 where network services are explicitly started
• Impact: Low risk - this is a standard service restart pattern already used elsewhere in the codebase

Potential Concerns:

• No obvious issue, but consider: Could calling SERVICES_UP when services are already running cause problems? Looking at the network services implementation, it should handle this gracefully, but it might be worth verifying that m_services->Start() is idempotent.

Fix #2: UI Update After Saving UPnP File State ✅

File: xbmc/utils/SaveFileStateJob.cpp:57-66

Change:
After successfully saving UPnP file state, the code now sends a GUI update message to refresh the UI with the new resume point.

Analysis:

• Problem Addressed: When playback stops on UPnP content, the UI wasn't updating to show the new resume point
• Solution: Creates an updated CFileItem with the correct resume point and path, then sends GUI_MSG_UPDATE_ITEM to the window manager
• Pattern Consistency: ✅ This mirrors the existing pattern at lines 179-196 for non-UPnP items
• Path Handling: ✅ Correctly handles both cases:
  • Uses original_listitem_url property if available (lines 60-61)
  • Falls back to progressTrackingFile (which should be the UPnP path) otherwise (line 63)

Implementation Quality:

• Creates a copy (updatedItem) rather than modifying the original - good practice
• Updates the video info tag's resume point before sending the message
• Uses std::make_shared<CFileItem> for the message parameter - correct smart pointer usage

Minor Observation:
The comment on line 63 is helpful: "fallback to progressTrackingFile which should be the upnp path"

Overall Assessment

Strengths:

1. Both fixes are minimal, targeted changes that address specific bugs
2. Code follows existing patterns in the codebase (consistency)
3. No breaking changes or API modifications
4. Proper use of existing infrastructure (NetworkMessage, GUI messages)

Code Quality:

• Clean, readable code
• Appropriate comments
• No memory leaks or resource issues
• Follows RAII principles with smart pointers

Testing Recommendations:
Since the PR template mentions testing wasn't fully documented, I'd recommend testing:

1. Fix XBMC core Portuguese translation based on English commit 1ee638d8b22a3a5440e57a9384590558925e12ef #1: On Android, verify UPnP servers are discoverable after backgrounding/resuming the app
2. Fix Confluence Portuguese strings translation update #2: Play a UPnP video, stop partway through, verify the resume point appears in the UI immediately

Approval: ✅
These are solid bug fixes that improve UPnP reliability. The code is safe, follows existing patterns, and addresses real user-facing issues. I see no blockers for merging.

@Kate6 now that we have both squishy and robot approvals, can you apply the Jenkins patch? Then we'll merge.

@Kate6 We want CI to be happy between each commit, so can you squash the third commit into the first and force-push? Jenkins checks each commit for style-correctness so that if one is reverted, we don't get an in-between result.

@Kate6 Oops, you squashed your two fixes. You can recover your work here: https://github.com/Kate6/xbmc/commits/b044731e9f3f59183d7cc6692915d3d483bb9815

Then the third commit should be squashed into the first.