Webkit2 page source view

[rti]

webkit2 does not provide the view mode source so maybe this is going to be removed together with the gf keybinding or we find a simple workaround for this

[stale]

This issue has been automatically marked as stale because it has not had activity within the last 60 days. It will be closed after 7 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity since it was marked as stale. Thank you for your contributions.

[fanglingsu]

There is now a possible solution to show the source by changing the HTML via JavaScript from within vimb on the mailing list https://sourceforge.net/p/vimb/vimb/message/36441932/.

[iamleot]

Daniel Carl writes:

There is now a possible solution to show the source by changing the HTML via JavaScript from within vimb on the mailing list https://sourceforge.net/p/vimb/vimb/message/36441932/.

Whoops... and I have missed this issue, sorry! So, feel free to discuss that here and/or on the MLs and I'll try to keep both discussions synced. Surely webinspector is less kludgy than the patch I have proposed and should not introduce any possible side effects. However, the rationale of having a working `gf' - also implemented in that way - is that if I need to quickly skim a web page source code and search on it via `/' the `gf' is usually faster than opening the web inspector IMHO. At the moment a limitation that I have observed - that in the web inspector can be workarounded by browsing the `Resources' tab is viewing the source for RSS/Atom feed, e.g.: <https://github.com/fanglingsu/vimb/releases.atom> In that case `gf' is actually a noop. Any feedback/review/critics/suggestions/improvements are welcomed!

[fanglingsu]

@iamleot There is not much traffic on the ML and I announce only new releases. Most of the communication is done via github. So maybe your mail will change this.

I liked the gf too and in fact it both, the weninspector and the source view where different. The webinspector shows the actual dom like it is parsed by webkit. This reflects also DOM-changes made by JavaScript and possibly by webkit. The source view showed the content like it arrived from webserver.
What do you think about fetching the sites html and to show it in the editor (set editor-command could be reused). So highlighting the source would be done by the editor. One drawback of this would be that the html is not indented but I think we can accept this.

[iamleot]

Hello Daniel! Daniel Carl writes:

@iamleot There is not much traffic on the ML and I announce only new releases. Most of the communication is done via github. So maybe your mail will change this.

Oh, yeah, I saw that, so, maybe we can just discuss about it there, thanks!

I liked the `gf` too and in fact it both, the weninspector and the source view where different. The webinspector shows the actual dom like it is parsed by webkit. This reflects also DOM-changes made by JavaScript and possibly by webkit. The source view showed the content like it arrived from webserver. What do you think about fetching the sites html and to show it in the editor (`set editor-command` could be reused). So highlighting the source would be done by the editor. One drawback of this would be that the html is not indented but I think we can accept this.

IMHO what you propose (using the editor) it's a better idea! I think that surf does something like that (but maybe with a patch?) and the solution is surely more elegant than what I propose. Feel free to beat me and/or I'll try to implement that in the next days! :) Thank you!

[fanglingsu]

@iamleot I haven't seen a patch for surf that allows to show the source in editor. But spawning external tools is the way suckless goes.
Feel free to work on this feature.

[iamleot]

normal_view_support.txt

Sure!

Here it is an initial and experimental version of it! Please note that most of the
code/logic was stolen from input_open_editor() (but I'm not sure if it's easy
to refactor both in order to share common code). Feedback/testing/comments
are welcomed, and of course feel free to ping me when you think it can be ready
to be shared as a pull request!

Thank you!

[fanglingsu]

@iamleot It would be nice if we can avoid doubling the code.
Maybe we can extract the common logic of both into a new function. I've used to write functions that are used from multiple modes in src/command.c. I think this function needs to get the content to show as string and optionally a callback similar to (GChildWatchFunc) and the data that are given to the callback.

gboolean command_spawn_editor(Client *c, const Arg *arg, GChildWatchFunc
callback, gpointer data)
{
    /* Check if editor-command is set */
    /* Write string arg->s to new temp file */
    /* spawn editor-command async */
    /* watch the child with own callback e.g. resume_editor (src/command.c) 
     * The given callback and data must be given in the data for the cild
     * watcher. */
}

static void resume_editor(GPid pid, int status, EditorData *data)
{
    /* Read the contents from the file */
    /* Call the callback out of editor data (Client* c, gpointer) */
    /* Free memory of the file contents */
    /* unlink the temporary file */
}

This is only my first thought about the way we could do it and it might be a
little more complicated in real. The input_open_editor needs to get the
form field value and give it to command_spawn_editor(). If this succeeds
the form field should be disabled. The callback mus be adapted, because this
will get the file contents instead of the current editor data.

[iamleot]

Hello Daniel, Daniel Carl writes:

@iamleot It would be nice if we can avoid doubling the code.

I agree!

Maybe we can extract the common logic of both into a new function. I've used to write functions that are used from multiple modes in `src/command.c`. I think this function needs to get the content to show as string and optionally a callback similar to (`GChildWatchFunc`) and the data that are given to the callback. ``` gboolean command_spawn_editor(Client *c, const Arg *arg, GChildWatchFunc callback, gpointer data) { /* Check if editor-command is set */ /* Write string arg->s to new temp file */ /* spawn editor-command async */ /* watch the child with own callback e.g. resume_editor (src/command.c) * The given callback and data must be given in the data for the cild * watcher. */ } static void resume_editor(GPid pid, int status, EditorData *data) { /* Read the contents from the file */ /* Call the callback out of editor data (Client* c, gpointer) */ /* Free memory of the file contents */ /* unlink the temporary file */ } ``` This is only my first thought about the way we could do it and it might be a little more complicated in real. The `input_open_editor` needs to get the form field value and give it to `command_spawn_editor()`. If this succeeds the form field should be disabled. The callback mus be adapted, because this will get the file contents instead of the current editor data.

Any suggestions regarding how to pass the filename used from command_spawn_editor() to resume_editor()? (I would like to implement something like that for this issue so that you can then reuse it to adapt input_open_editor()/resume_editor() in input.c). IIUC `*data' should be untouched by command_spawn_editor and just passed as-is to resume_editor(). Thank you!

[fanglingsu]

@iamleot That the parameter of both functions are named data is a little bit misleading. The command_spawn_editor data parameter is untouched and this function does not need to know what it is. The data parameter of resume_editor is a new structure that needs to be known to only these two functions. This structure holds the file path (to remove the file), the callback and the gpointer that were given to command_spawn_editor (to call the use case specific callback - do nothing in case of the source view or put the text into the form field in case of the existing form field editing).

[iamleot]

Thank you! Here preliminary refactor of (hopefully!) what was discussed. Any feedback/comments welcomed! https://github.com/fanglingsu/vimb/files/2493790/normal_view_support_v2.txt

[fanglingsu]

@iamleot I think it would be easier when you do this in a separate branch. So we can discuss and comment on single lines or commits.

I thought of adding a resume function to src/command.c too which is responsible to g_spawn_close() the pid and unlinking the temporary file. But bevore that, this function should read the file contents and give them to the original callback (but only if a callback was given). So the callbacks do not need to know if the data came from a file or where the file is located. So the EditorData struct can be moved out of main.h into command.c because this is only used by the two function in command.c.
So we need some typedef for the callback in main.h. I think we need at least the contents of the file (must be freed in the callback after use) and the Client.

[iamleot]

Hello Daniel, Daniel Carl writes:

@iamleot I think it would be easier when you do this in a separate branch. So we can discuss and comment on single lines or commits.

Good idea! I'll probably commit and push a separate branch in the next few hours.

[iamleot]

I have committed and pushed in view_source_support branch (and, it
contains also further clean ups compared to the patch previously shared here).

Any comments/reviews are welcomed!

[iamleot]

I thought of adding a resume function to src/command.c too which is responsible to g_spawn_close() the pid and unlinking the temporary file. But bevore that, this function should read the file contents and give them to the original callback (but only if a callback was given). So the callbacks do not need to know if the data came from a file or where the file is located. So the EditorData struct can be moved out of main.h into command.c because this is only used by the two function in command.c.
So we need some typedef for the callback in main.h. I think we need at least the contents of the file (must be freed in the callback after use) and the Client.

Regarding a resume function to src/command.c, IMHO at least at the moment it is not worth the effort and probably will not ease any reusability of the code. The "cleanup" (g_free() and similar) of resume_editor() function are just 2-3 LOCs and resume_editor() function are already pretty customised in the context where they are used.

[fanglingsu]

@iamleot I did not though in LOC for the resume functions but in responsibilities. The caller od the edior command does not know anything about a file and can therefore not be responsible for unlinking it. So we don't add a global structure to provide internals that are only used in src/command.c.
We can't expect each caller to implement a resume function to unlink the file and free some memory and do some child close pid stuff.

[iamleot]

Daniel Carl writes:

@iamleot I did not though in LOC for the resume functions but in responsibilities. The caller od the edior command does not know anything about a file and can therefore not be responsible for unlinking it. So we don't add a global structure to provide internals that are only used in `src/command.c`. We can't expect each caller to implement a resume function to unlink the file and free some memory and do some child close pid stuff.

OK! Can you please make comments in the proposed code? (I think this will ease implementing that or, if it looks okay please let me know and I'll request a pull up so you can then adjust the code) Thank you!

[fanglingsu]

@iamleot I've added what I've already written into the commits. Hope this makes it easier to understand.
Thank you that you spent so much time contributing to vimb!

[iamleot]

Daniel Carl writes:

@iamleot I've added what I've already written into the commits. Hope this makes it easier to understand. Thank you that you spent so much time contributing to vimb!

Yes, I hope I will be able to address all the comments ASAP and ping you back when all is addressed or if I will need any feedback! Thank you a lot!

[iamleot]

I have readjusted normal_view_source() to directly retrieve the data via
webkit_web_resource_get_data() instead of injecting JavaScript and inspecting
the DOM. Now RSS/Atom feed sources can be seen as well via gf.

[iamleot]

Hopefully I've addressed all comments and I think it is in a state to request a possible pull request.

If there are any further possible changes to do I would like to reorder a bit the commits and open a pull request.

[fanglingsu]

@iamleot It would be nice if you would squash commits together into some few commits. I don't think we need this many commits that fix indentation. Put together what belongs together.

[fanglingsu]

@iamleot Thank you very much for your work. This works fine!

[iamleot]

Thank you again for kindly reviewing multiple version of the patch, for all the suggestions and for merging it!