Oof, this block is hard to understand. 🙂

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.

The pageNode is never used.

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

There’s also a typo in the initalContent.

The exceptions aren’t thrown anywhere.

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.