Fix diff mode bug

[aadcg]

Description

No luck still... Something beyond my understanding is going on here. I need help! Please read commit 31956cf52 for more details.

I also need to implement this feature.

CC @Ambrevar @aartaka

(Will) fix #2328.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• [x] I have pulled from master before submitting this PR
• [x] My code follows the style guidelines for Common Lisp code
  • See Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF) and Google Common Lisp Style Guide.
• [x] I have performed a self-review of my own code
• [x] My code has been reviewed by at least one peer (the peer review to approve a PR counts. The reviewer must download and test the code)
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have made corresponding changes to the documentation
• [x] My changes generate no new warnings
• [x] I have added tests that prove my fix is effective or that my feature works
• [x] New and existing unit tests pass locally with my changes
• [x] There are no merge conflicts

[aadcg]

@aartaka, indeed I was doing it wrong, but that was because I was trying to solve the real problem that I identified before.

Let me describe it.

(define-internal-page-command-global testing
    (&key (buffer-id (id (prompt1
                           :prompt "A buffer"
                           :sources (make-instance
                                     'buffer-source
                                     :multi-selection-p nil
                                     :return-actions nil)))))
    (test-buffer "*test*" 'base-mode)
  "Test"
  (spinneret:with-html-string
    (:raw (ffi-buffer-get-document (nyxt::buffers-get buffer-id)))))

This doesn't work and the problem lies at the guts of internal pages (namely the macro with-current-buffer). I'm afraid I'm still not able to fix this...

[aadcg]

Found it. The culprit is commit e176a24636a9377a3af9b931740efb4a95a8b5a8.

To make sure that we get this working again, test on internal pages, not web buffers.

(define-internal-page-command-global testing
    (&key (buffer-id (id (prompt1
                           :prompt "A buffer"
                           :sources (make-instance
                                     'buffer-source
                                     :multi-selection-p nil
                                     :return-actions nil)))))
    (test-buffer "*test*" 'base-mode)
  "Test"
  (spinneret:with-html-string
    (:raw (ffi-buffer-get-document (nyxt::buffers-get buffer-id)))))

CC @Ambrevar @aartaka

I don't know how to fix this.

Even when we fix it, I think we'll have another issue. But one thing at a time :D

[aadcg]

@aartaka had a brief look at this but this seems to be a tough one. We'll be having a look at this together next week. I'm be taking the driver's seat and @aartaka will be the co-pilot.

[Ambrevar]

We use numerical IDs now with 962311d067816de653d1f88ce10f3a02f0b8b552. Does this help?

[aadcg]

@Ambrevar indeed we now have part of the problem solved.

Unfortunately, as I feared, there's still a nasty memory error when running the code below on any web buffer (NOT internal pages):

(define-internal-page-command-global testing
    (&key (buffer-id (id (prompt1
                           :prompt "A buffer"
                           :sources (make-instance
                                     'buffer-source
                                     :multi-selection-p nil
                                     :return-actions nil)))))
    (test-buffer "*test*" 'base-mode)
  "Test"
  (spinneret:with-html-string
    (:raw (ffi-buffer-get-document (nyxt::buffers-get buffer-id)))))

Notice, however, that the following works:

(spinneret:with-html-string (:raw (ffi-buffer-get-document (buffers-get (id (current-buffer))))))

Which indicates that the issue seems to be on our side...

[aadcg]

@Ambrevar I believe #2354 wasn't enough to address the issue I mention in the comment above.

[Ambrevar]

I'll come back to this when I'm done with the preparation for 3-pre-release-1, sorry for the delay!

[aadcg]

@Ambrevar no need for your intervention for now :)

I know how to fix this.

The issue related to internal pages has been fixed by you on #2398. Thanks!

[aartaka]

@aadcg, mind making this ready-for-review so that I can push changes too?

[aadcg]

Done @aartaka.

[aartaka]

@aadcg, I've fixed relative URL resolving in 9bbce3b52ce0413873dd6af40c007a3de4f299b8, but I want to add some more API niceties, so don't rush merging :)

[aartaka]

Ready for review now. Manual and changelog updated, diff-mode works for both open buffers and new URLs, spawning the background buffer to fetch the sources if necessary. I like the API and the look of diff: URLs. However, an additional pair of eyeballs won't hurt :)

@aadcg, how does this variation on the topic of diff-mode look to you?

[aartaka]

All addressed except for once-on discussion, as I am still uncertain about its status :)

[aartaka]

I'm too used to the fact that we tend to use more wordy yet explicit code idioms that the convenient and short ones. The fact that there are both short and explicit idioms (because of macros' power) was not obvious to me xD

[aadcg]

@aartaka how did you test this?

If I request a diff between https://github.com/atlas-engineer/nyxt/ and https://github.com/atlas-engineer/nyxt/tree/1.5.0, it does wild things.

[aadcg]

The open-urls-source is evil since if you're at URL prefix and want to go to prefix-foo, it won't let you.

[aartaka]

@aartaka how did you test this?

If I request a diff between https://github.com/atlas-engineer/nyxt/ and https://github.com/atlas-engineer/nyxt/tree/1.5.0, it does wild things.

I haven't used it before it broke, so I was unsure about how good it should be :sweat_smile:

[aartaka]

OK, it seems that html-diff produces some HTML that Plump cannot parse and serialize correctly. Several solutions I see there:

• Use one of the diffed URLs as the host for diff: URLs so that WebKit resolves the relative URLs against it.
• Use WebKitURISchemeResponse and redirect sub-resources to the real ones, saving us time and loads.
• Somehow parse the results of html-diff and insert style and base tags there.
• Rewrite html-diff using Plump (should save lots of lines and be more compatible).

[Ambrevar]

• Use one of the diffed URLs as the host for diff: URLs so that WebKit resolves the relative URLs against it.

How is it related to the problem that Plump cannot parse html-diff results?

[aartaka]

• Use one of the diffed URLs as the host for diff: URLs so that WebKit resolves the relative URLs against it.

How is it related to the problem that Plump cannot parse html-diff results?

It avoids parsing (or doing anything at all) with html-diff results and thus kind of solves the problem. Although, obviously, a more straightforward solution would be to either

• Make Plump a bit more tolerant to the malformed HTML html-diff produces, or
• Make html-diff produce a bit more structured HTML.

[aadcg]

It seems that diff-mode will be left out of Nyxt for 3.0 so this should be closed.

[Ambrevar]

Wait, isn't a first attempt at fixing the issue? Even if we don't merge it for 3.0, we can keep this open until it's properly fixed, right?

[aadcg]

Wait, isn't a first attempt at fixing the issue?

Sorry, could you rephrase?

Even if we don't merge it for 3.0, we can keep this open until it's properly fixed, right?

You're suggesting that it could be re-integrated after 3.0 once we fix it, so it could stay open right? I agree then.

[Ambrevar]

Yes!

[aartaka]

@aadcg, can you test? I've tried fixing with <base> tag addition and it seems to more or less work.

[aadcg]

How did you test it?

It doesn't produce satisfactory results as those reported in the old article.

[aartaka]

Ahhh, it seems that the problem lied much deeper than <base> or Plump-based parsing. Seems like our current scheme architecture doesn't allow us to trick WebKit into resolving the relative links against the page we clearly are not. Security, security :D

[aartaka]

At this point, I'm no longer sure we can retain the functionality that diff-mode used to have, except maybe for simple HTML-only pages...

EDIT: pageS

[aadcg]

That was my point all along :)

The mode is trivial. It stopped working when we deprecated internal buffers where you could say "here's the HTML, render it". Those were deprecated for security reasons obviously. My only hope was that by fine-tuning the parameters WebKitGTK provides for custom schemes perhaps we could bring it back to life.

I'm in favor of forgetting about it. It was devised in completely different circumstances. I think the concept is worth exploring in the future, it is just that we need another completely different strategy. This is post 3.0.

[aartaka]

Agreed (˘⌣˘)