Quote n2ntms when replying #1043

ArneBab · 2025-03-03T21:52:27Z

No description provided.

src/freenet/node/useralerts/UserAlertManager.java

src/freenet/node/useralerts/UserAlert.java

src/freenet/node/useralerts/UserAlertManager.java

src/freenet/node/useralerts/UserAlert.java

ArneBab · 2025-06-24T08:19:53Z

resolved conflicts with next + force pushed.

Bombe · 2025-06-24T08:28:54Z

src/freenet/node/useralerts/DownloadFeedUserAlert.java

 import freenet.support.HTMLNode;

-public class DownloadFeedUserAlert extends AbstractUserAlert {
+public class DownloadFeedUserAlert extends AbstractUserAlert implements UserAlertFromPeer {


In my opinion, this is going into the wrong direction. You are putting more and more content into the user alerts, but it doesn’t belong there. If you get a message from someone, that message should contain everything you need. User alerts are for showing, well, alerts. You can totally generate a user alert from e.g. a message that you have received, but none of the actual message data should be included in the alert.

From what I can see, there is no message. The Alerts are generated directly from SFS’s in the peer notes. We currently have this double-structure where we can answer some alerts and others we cannot, so this actually are two different concerns, munged into one structure. There are even some we can accept or reject (file offers).

We could disentangle that into different objects with actual data handling (instead of managing data in what’s effectively UI objects) that get dispatched by type, but that requires more refactoring and it’s not what I currently want to achieve: my current short-term goal for next release is polishing user interface warts in friend-to-friend workflows.

But I see extracting dedicated interfaces as a path towards cleaning up the structure: once the different types of alerts are represented by interfaces we can more easily swap in proper data handling (and for example have dedicated peer communication commands via FCP).

From what I can see, there is no message. The Alerts are generated directly from SFS’s in the peer notes. We currently have this double-structure where we can answer some alerts and others we cannot, so this actually are two different concerns, munged into one structure. There are even some we can accept or reject (file offers).

That is exactly what I see as biggest part of the problem. N2NTMs are not entities in their own right, they have been violently shoe-horned into a system that was not made for them, and the thing we absolutely should not do is entrench the notion that it’s okay for the messages (or any other actual data, for that matter) to live in the user alerts.

We could disentangle that into different objects with actual data handling (instead of managing data in what’s effectively UI objects) that get dispatched by type, but that requires more refactoring and it’s not what I currently want to achieve: my current short-term goal for next release is polishing user interface warts in friend-to-friend workflows.

So you are intentionally increasing technical debt for a (questionable!) short-term goal, making it harder for us to ever fix the underlying problem? That’s quite rude! 🙂

(Why do I think your goal is questionable? Because in my opinion the biggest problems N2NTMs face – besides the name 😬 – is that nobody knows they exist, and if somebody does know they exist, they don’t know what to do with them. Adding “quote on reply” to functionality that is already obscure at best will do nothing to further its adoption, but it definitely will do is make it harder for us to actually separate N2NTMs – we need to fix this name – and user alerts, as well as everything else that is currently existing only in user alerts, making all of it available with a well-defined interface in the node and via FCP, so that you can use different clients to view and manage your N2NTMs.)

But I see extracting dedicated interfaces as a path towards cleaning up the structure: once the different types of alerts are represented by interfaces we can more easily swap in proper data handling (and for example have dedicated peer communication commands via FCP).

As long as all the data is living in the user alerts, you will never have this separation, and forcing it on clients to use instanceof everywhere in their code just to know if this random user alert does maybe contain a message is quite the nasty code smell.

The only forward here (again, in my opinion) is to elevate N2NTMs to be an entity in their own right, with its own in-node infrastructure and FCP interface, and then the functionality you are trying to add here will only touch the N2NTM management code and not the user alerts.

I think the notion that no one knows that n2ntms exists is wrong. I’ve been seeing problem reports about them for years and people with darknet peers are actively using them to share bookmarks and to write messages. My first fix to the n2ntm’s was in 2018 (to lift the 1k size restriction). I think the reason we’re not seeing those reports far more often is that few people use darknet. Which is partly because our one distinguishing feature -- truly confidential messages -- has lots of UI warts.

The naming is fixed in the UI already ("send confidential message").

What I’m doing therefore is to extract a minimal interface so it doesn’t matter for the touched UI code where the data lives or whether the object is a UserAlert at all. That way we can split off storage of node messages from user alerts and only combine them in the alerts UI (for continuity), so other places can more easily get only the node to node messages (I need those as separate list in the core operations interface I’ve already been planning for too long).

I think we have to move to a better structure incrementally, because other approaches regularly fail either in getting stuck or in overloading reviewers.

… maybe the Interface should then not be called UserAlertFromNode but something like MessageFromNode or FriendNote (though message is already taken for FCP message -- do you have a better suggestion?).

I think the reason we’re not seeing those reports far more often is that few people use darknet. Which is partly because our one distinguishing feature -- truly confidential messages -- has lots of UI warts.

It’s hard to know which one is the cause and which one is the symptom. 😀

What I’m doing therefore is to extract a minimal interface so it doesn’t matter for the touched UI code where the data lives or whether the object is a UserAlert at all. That way we can split off storage of node messages from user alerts and only combine them in the alerts UI (for continuity), so other places can more easily get only the node to node messages (I need those as separate list in the core operations interface I’ve already been planning for too long).

I would absolutely prefer if you would do that first… 🙂

I think we have to move to a better structure incrementally, because other approaches regularly fail either in getting stuck or in overloading reviewers.

We absolutely should do that, but that change is a move into the wrong direction, in my opinion, because you are making the structure worse.

… maybe the Interface should then not be called UserAlertFromNode but something like MessageFromNode or FriendNote (though message is already taken for FCP message -- do you have a better suggestion?).

I don’t have strong feelings either way about the name of this interface; I believe its existence to be a mistake. 🙂

Core Problem is: I can’t get such a refactoring done in time for the next release (possibly next weekend), and I would really like to get communication with n2n messages more usable for the next release. If you see something that can be done without risk in a very short amount of time, I can try, otherwise I would really prefer to first get the UI improvement in and work on the structure in the background when I take up #846 again (which is the first larger change I want to do after the release).

That needs messages by peers separate from UserAlerts.

And yes, cause and symptom are hard to distinguish.

Core Problem is: I can’t get such a refactoring done in time for the next release (possibly next weekend), and I would really like to get communication with n2n messages more usable for the next release.

Using a timelimit you set yourself as a reason to get your change into Fred feels like strong-arming. 😄

If you see something that can be done without risk in a very short amount of time

That kind of implies that your change is without risk, which it decidedly is not.

I would really prefer to first get the UI improvement in and work on the structure in the background when I take up #846 again (which is the first larger change I want to do after the release).

Adding a function in a place as hidden as the list of 800 minor warnings is not a UI improvement; it makes our UI harder to use! An actual UI improvement would be to a) fix the name of N2NTMs (every time I’m thinking the name my brain stumbles), b) add a section in the menu bar that deals only with those messages, and c) expose them via other UIs (such as FCP).

I can totally understand that you want this change to go in, and I can’t stop you, but I don’t think it’s a good change; while the intention may be pretty cool, the implementation currently does not allow this change to be made without fucking things up more than before.

I’m sorry that it appears like strong-arming. The timelimit is not because I particularly like the next weekend, but because it is likely the only weekend in the next months that I can mostly use freely.

But since this disturbs you that much, I’ll leave this PR out of the next release. As long as there are no new vulnerabilities we have to fix before making the code public, release 1503 should be doable without needing a dedicated weekend for it, so we should be good then, hopefully with this PR cleaned up sufficiently for inclusion.

I’m sorry that it appears like strong-arming. The timelimit is not because I particularly like the next weekend, but because it is likely the only weekend in the next months that I can mostly use freely.

But since this disturbs you that much, I’ll leave this PR out of the next release. As long as there are no new vulnerabilities we have to fix before making the code public, release 1503 should be doable without needing a dedicated weekend for it, so we should be good then, hopefully with this PR cleaned up sufficiently for inclusion.

You completely misunderstood the points of my last message. It’s not about the timelimit, it’s about that you are using the limit you yourself set as a reason to speed up integration of this PR. And it’s not about the form of this PR, it’s about what you are trying to do in the PR.

I understood that. And with "form" of this PR I meant adding to UserAlerts instead of refactoring first.

Methods: - canReply() — show reply button? - getSourceNode() — whom to reply to - getMessageText() — the content - replyButtonText() — custom label per Alert Goals: - Enable replying with a POST request (contains the form password). - Turn reply into a standard operation for N2NTMs instead of an implementation detail of the specific User Alerts. Only the N2NTMUserAlert implements these for now.

The message needs escaping of linebreaks, because those get lost in hidden input elements.

Bombe · 2025-09-03T18:22:06Z

src/freenet/clients/http/N2NTMToadlet.java

+				message = "\n> " + String.join("\n> ", message.split("\n")) + "\n\n";
+				message = message.replaceAll("\n> >", "\n>>")
+						.substring(1); // strip the initial linebreak again


Oof, this block is hard to understand. 🙂

Suggested change

message = "\n> " + String.join("\n> ", message.split("\n")) + "\n\n";

message = message.replaceAll("\n> >", "\n>>")

.substring(1); // strip the initial linebreak again

message = Arrays.stream(message.split("\n"))

.map(line -> "> " + line)

.map(line -> line.replaceFirst("^> >", ">>"))

.collect(joining("\n")) + "\n\n";

This is straight from the hip, so it might need a reality check, but this captures what (I think!) you’re trying to do quite nicely.

Bombe · 2025-09-03T18:53:46Z

src/freenet/node/useralerts/UserAlert.java

+	default public String getMessageText() {
+		return getText();
+	}


Uhm… that’s a third method now that returns the text of the user alert. Why do we need a third method for this? All (one) places this is used from have a N2NTMUserAlert instance which implements the UserAlertFromPeer interface, giving us access to this method. There is no reason for further polluting UserAlert.

Bombe · 2025-09-03T18:54:40Z

src/freenet/node/useralerts/UserAlert.java

 package freenet.node.useralerts;

 import freenet.clients.fcp.FCPMessage;
+import freenet.node.PeerNode;


I wonder what this import is about, as apparently no other change in this file even so much as thinks of mentioning PeerNode… 😁

ArneBab · 2025-09-03T20:42:18Z

Sorry for triggering you to review again. I only fixed merge conflicts. I’ll write here once I actually improved the code instead of just brushing away bitrot.

Bombe · 2025-09-04T05:40:27Z

src/freenet/node/useralerts/UserAlertFromPeer.java

+public interface UserAlertFromPeer {
+
+	public PeerNode getSourceNode();
+    public String getMessageText();


You know, the more I think about it, the more I think that this method should mirror the one that already exists on UserAlert. As these two interfaces seem to be kind of linked, they can probably share methods, too. 🙂

Bombe · 2025-09-04T05:41:44Z

src/freenet/node/useralerts/UserAlertManager.java

 		alertContentNode.addChild(userAlert.getHTMLText());
+		if (userAlert instanceof UserAlertFromPeer) {
+			UserAlertFromPeer n2nUserAlert = (UserAlertFromPeer) userAlert;
+			alertContentNode.addChild(renderReplyButton(n2nUserAlert, n2nUserAlert.getSourceNode()));


If you are already handing in the UserAlertFromPeer, why are you extracting the source node and handing that in separately?

Bombe · 2025-09-04T05:43:13Z

src/freenet/node/useralerts/UserAlertManager.java

+					new String[]{"true", "peernode_hashcode", peerNode == null
+							? ""
+							: Integer.valueOf(peerNode.hashCode()).toString()});


Hmm, I’m wondering, if we don’t have a peer node, who are we sending the reply to?

✅ Add first test for N2NTM reply form

Bombe · 2025-09-07T16:24:34Z

src/freenet/clients/http/N2NTMToadlet.java

 			throws ToadletContextClosedException, IOException {
+		createN2NTMSendForm(null, advancedMode, contentNode, ctx, peers, "");
+	}
+	public static void createN2NTMSendForm(HTMLNode pageNode, boolean advancedMode,


The pageNode is never used.

Bombe · 2025-09-07T16:24:47Z

src/freenet/clients/http/N2NTMToadlet.java

+	}
+	public static void createN2NTMSendForm(HTMLNode pageNode, boolean advancedMode,
+			HTMLNode contentNode, ToadletContext ctx, HashMap<String, String> peers, String initalContent)
+			throws ToadletContextClosedException, IOException {


The exceptions aren’t thrown anywhere.

Bombe · 2025-09-07T16:25:47Z

src/freenet/clients/http/N2NTMToadlet.java

 			throws ToadletContextClosedException, IOException {
+		createN2NTMSendForm(null, advancedMode, contentNode, ctx, peers, "");
+	}
+	public static void createN2NTMSendForm(HTMLNode pageNode, boolean advancedMode,


And can I convince you to make this method private, because there’s no reason it shouldn’t be? 😁

Bombe · 2025-09-07T16:26:17Z

src/freenet/clients/http/N2NTMToadlet.java

+		createN2NTMSendForm(null, advancedMode, contentNode, ctx, peers, "");
+	}
+	public static void createN2NTMSendForm(HTMLNode pageNode, boolean advancedMode,
+			HTMLNode contentNode, ToadletContext ctx, HashMap<String, String> peers, String initalContent)


There’s also a typo in the initalContent.

bertm reviewed May 10, 2025

View reviewed changes

src/freenet/node/useralerts/UserAlertManager.java Outdated Show resolved Hide resolved

bertm reviewed May 10, 2025

View reviewed changes

src/freenet/node/useralerts/UserAlertManager.java Outdated Show resolved Hide resolved

bertm reviewed May 10, 2025

View reviewed changes

src/freenet/node/useralerts/UserAlert.java Outdated Show resolved Hide resolved

bertm reviewed May 10, 2025

View reviewed changes

src/freenet/node/useralerts/UserAlertManager.java Outdated Show resolved Hide resolved

bertm reviewed May 10, 2025

View reviewed changes

src/freenet/node/useralerts/UserAlert.java Outdated Show resolved Hide resolved

ArneBab force-pushed the quote-n2ntms-when-replying branch from 65b6f51 to f541ce7 Compare June 24, 2025 08:19

Bombe reviewed Jun 24, 2025

View reviewed changes

ArneBab added 14 commits September 2, 2025 18:54

Extract createWriteN2NTMForm and add initialContent parameter

cddca54

Migrate n2ntm reply to POST and quote the original message

8e2bea3

The message needs escaping of linebreaks, because those get lost in hidden input elements.

Replace single-use WeakReference by node itself.

d9f6390

add null-safety

8c61739

doc: add reason for N2NTMUserAlert

fe052e3

refactor: remove unneeded reply methods from UserAlert. Thanks to bertm!

fdc8ea0

Encode with base64 instead of ad-hoc

80e3473

remove unused import

299fcbf

make renderReplyButton private

3dc8bc7

remove unused canReply method

614bd95

fix: rename translation key to the new location

e4d40ae

Support replying to proposed file

280af66

Fix rebase error

c69b7dc

ArneBab force-pushed the quote-n2ntms-when-replying branch from 55b62a6 to c69b7dc Compare September 2, 2025 16:58

Bombe reviewed Sep 3, 2025

View reviewed changes

Bombe reviewed Sep 4, 2025

View reviewed changes

Bombe and others added 2 commits September 6, 2025 16:30

✅ Add first test for N2NTM reply form

14455f5

Merge pull request #7 from Bombe/tests/quote-n2ntms-when-replying

e163ed1

✅ Add first test for N2NTM reply form

Bombe reviewed Sep 7, 2025

View reviewed changes

Bombe mentioned this pull request Nov 9, 2025

Fix: open as text link on downloads now uses utf-8 encoding #1109

Open

-				message = "\n> " + String.join("\n> ", message.split("\n")) + "\n\n";
-				message = message.replaceAll("\n> >", "\n>>")
-						.substring(1); // strip the initial linebreak again
+                message = Arrays.stream(message.split("\n"))
+                    .map(line -> "> " + line)
+                    .map(line -> line.replaceFirst("^> >", ">>"))
+                    .collect(joining("\n")) + "\n\n";

Quote n2ntms when replying #1043

Are you sure you want to change the base?

Quote n2ntms when replying #1043

Uh oh!

Conversation

ArneBab commented Mar 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArneBab commented Jun 24, 2025

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!

ArneBab Jun 24, 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!

ArneBab Jun 24, 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!

Bombe Jun 26, 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!

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

Uh oh!

ArneBab commented Sep 3, 2025

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArneBab Jun 24, 2025 •

edited

Loading

ArneBab Jun 24, 2025 •

edited

Loading

Bombe Jun 26, 2025 •

edited

Loading