[Geany-Devel] splitwindow2

[Matthew Brush]

On 13-10-10 05:03 PM, Thomas Martitz wrote:
> Am 11.10.2013 00:55, schrieb Matthew Brush:
>> On 13-10-10 03:03 AM, Thomas Martitz wrote:
>>> Hello,
>>>
>>> I just wanted to let you know that I'm working on a new splitwindow
>>> implementation and I would like to have early input. But, also as a
>>> warning, I'm progressing only slowly because I'm at the final phase of
>>> my master thesis which currently needs more attention that Geany
>>> hacking.
>>>
>>> Anyway, we all know about the limitations of the splitwindow plugin.
>>> They include (not a complete list):
>>> - the other view can only have one document
>>
>> You can actually change which document it shows by using the little
>> drop-down button at the top. Still, real notebooks would be nicer.
>
> Yea, right. I worded it badly. I meant the other view can only hold one
> document and you can swap that one only using non-standard methods.
>

Yeah, it's a kind of weird interface, although it works not bad considering.

>>
>>> - you cannot undo/redo: you need to select the tab in the main notebook
>>> to do that
>>
>> You can undo using the builtin Scintilla right-click context menu, but
>> still would be great to have full keybindings+edit menu functionality.
>
> Oh, I wasn't aware of that. Still crap :)
>

It was somewhat recent-ish. At first we didn't have it because it didn't 
integrate into gettext translation stuff, so we couldn't translate the 
menu items, but IIRC it was later decided that it was such a needed 
feature being able to copy/paste/undo/etc, that at least having a 
minimal non-translated menu is better than nothing.

>>
>>> - state is not saved/restored across Geany restarts
>>> - it's completely awkward because the other view shows a doc that's also
>>> in the main view, editing in the other view will change the main view at
>>> the same time.
>>
>> I think this is intended and I find it useful personally. I don't
>> think it should be taken out, IMO.
>
>>
>>> - you cannot toggle between the views with a keybinding (e.g. ctrl+tab)
>>> - weird focus behavior
>>> - more...
>>
>> ...Basically all edit keybindings, the edit/context menu and also
>> auto-indentation and similar features implemented inside Geany.
>>
>>>
>>> My plan is to reimplement splitwindow using a different approach: by
>>> having two real, independemt notebooks, one primary and one seconary.
>>>
>>
>> Why limit it to two? Even if it ends up being the default/primary mode
>> of operation, IMO it'd be a shame to make another hard-coded
>> assumption.[1]
>
> Well, first: I'm not going to be hardcode two notebooks (except where
> necessary and trivial extend, e.g. for my current "toggle this between

This sounds good.

> notebooks". 99% of the now hardcoded places will work fine, e.g. I'm
> using gtk_widget_get_parent(sci) to get a doc's notebook and implement a
> foreach_notebook() macro. So this should make it really trivial to
> support even more than 2 notebooks.
>

gtk_widget_get_parent(sci) makes a big assumption; that the Scintilla 
will be packed directly into a notebook. Plugins can easily break this 
assumption by reparenting the Scintilla widget, and even my 
document-messages branch moved each Scintilla into a VBox inside each 
tab so it could pack a GtkInfo bar above it. I think we could get rid of 
this assumption by designing the code better.

Please don't write another foreach_ macro. Lets make it so we can use 
normal C code. If you need a macro like foreach_notebook() it's probably 
because the code it's hiding is crappy underlying code, IMO.

> But still, I personally don't have a need for more than 2 notebooks.
> Plus I see it will be difficult to support, because you need to nest
> GtkPaneds. Since you suggest arbitrary splits you can do this only
> programmatically which sounds like a major headache to me (especially
> w.r.t. saving/restoring state). For now I want to concentrate on lifting
> the hardcoded single notebook without ending up in the same situation
> for two ones and without worrying about the heavy case of arbitrary
> notebook placing. Then we can continue from there on with more
> sophisticated stuff.
>
> However if you have already successfully implemented this in a toy
> editor I welcome you to join the fun.
>

Just to be clear, regarding the split window feature in my toy editor, I 
only coded the UI logic, not any of the storing config of what panes 
were open or what documents they had in them and more complicated stuff 
like this. But the UI was indeed quite neat. SublimeText also has a 
similarly flexible split pane system.

>
>>
>>> New docs will be opened in the primary one by default, but can be moved
>>> to the secondary one at will. This means that no document can be shown
>>> in both at the same time which makes it a loss less awkward. It also
>>
>> As mentioned above, this is actually a feature and IMO it should be
>> kept unless there is some serious technical reason that makes it
>> impossible. Scintilla can easily be showing the same document in two
>> views and it keeps them in sync (see SCI_SETDOCPOINTER and friends).
>
>
> Do you edit on both views or just read on one part of the doc while
> editing the other? If this works automagically via scintilla then fine
> but I can imagine it has more implications/complications apart from
> Scintilla. Thus for me this has low priority so I would like to get the
> non-duplicate docs use case working first.
>

Well for the Scintilla part, it "just works", it's exactly what it was 
designed for. I'm not sure about any of the other complications apart 
from Scintilla. Either way, as long as we don't make any assumptions 
that two views can't be displaying the same document, it's no big deal 
for now.

>>
>>> lets us lift the other limitations:
>>> - undo/redo will work
>>> - state can be saved across restarts (config file format will probably
>>> need some changes for this)
>>> - toggling with keybindings between views can work
>>> - you can arbitrarily assign docs to views using different methods
>>>
>>> The nice things about splitwindow that should be probably be kept:
>>> - chose between horizontal and vertical split
>>> - automatic sizing of the views
>>
>> Just to be clear, because Lex's message made it sound like it was
>> slightly magic; it's just the width/height available divided by two
>> right now IIRC (which is fine IMO). With your above item about saving
>> state, hopefully it would include the splitter positions, but I guess
>> it's not a big deal if not.
>
> Automatic sizing also includes what to do if one or both of the
> notebooks becomes empty, a previously empty one becomes non-empty or how
> to deal with the user making non-50/50 splits. Although I'm not even
> sure the current splitwindow handles all cases well?
>

When the last tab is closed, close the split. If it's easy to re-split, 
it doesn't matter. Like user just opens the document, right clicks on 
the tab and presses the "move to split->vertical" menu item or something 
like this. As mentioned in the other message, it probably isn't so 
important to care about splitter positions, as long as they default to 
50/50, at least for now.

>>
>>> - anything else?
>>>
>>
>> Even though I usually rant against hard-coding language specific stuff
>> into the core, I think it would be extremely useful to have an option
>> to open header/implementation file in the other view. For example if
>> I'm editing foo.c, it could look for foo.h (either being open or in
>> same directory), and open it in the other view. It could work the
>> other way around as well. I think it'd probably only be really useful
>> for C, C++ and Obj-C filetypes, although I don't know that many
>> languages, so aybe some others could benefit as well.
>
> This is provided by two plugins already. I think those plugins should be
> able to show the header on the other notebook once they are adapted for
> multiple notebooks. I also think of other use cases, e.g. geanyvc
> opening the file diff/log/blame of the current file in the other notebook.
>

You are correct, I agree.

>>
>>> Unfortunately Geany's core is pretty hardcoded as to assume one (and
>>> only one) notebook for documents. This means that for this various
>>> changes to Geany core are necessasry, some of which may be difficult to
>>> do without breaking the plugin API/ABI. Some are gonna hate this but I'm
>>> not currently planning do this as a plugin because the core needs a lot
>>> of changes anyway for breaking the single notebook assumption. But I'm
>>> very open for discussion which is why I'm writing this early.
>>>
>>
>> I agree it should be a core feature and a lot of users (myself
>> included) consider it a critical feature. I think it could be done
>> without breaking too much in the plugin API except a few functions
>> like document_compare_by_tab_order() and document_get_notebook_page()
>> and stuff, but maybe I'm missing something.
>
>
> One problem is that there are some document_* functions which only refer
> to page numbers. These are ambiguous with two (or more) notebooks. So
> plugins that call this might act up. For now I changed these functions
> to use the notebook that's currently focused but I of course added
> corresponding functions with an explicit notebook parameter too.
>

All of those functions in document_ module shouldn't care the least 
about GTK+ or even notebooks, IMO. This is the type of stuff I want to 
cleanup.

> There are also many direct uses of main_widgets.notebook, changing these
> might (or might not) affect plugins.
>

IMO, if you're going to break the plugin API anyway, I say make it good 
and we'll fixup core and GP plugins and make a message on the mailing 
list for other users to know what broke and how to fix it in their own 
personal plugins.

>
>>
>>> I already have an experimental version up and running that doesn't even
>>> require all that much changes[1] and it seems to work nicely. See
>>> this[2] screenshot.
>>>

IMO, least amount of changes is not necessarily a virtue, and can even 
lead to further gluing of the code together making future changes even 
harder.

>>> So, any opinions?
>>>
>>
>> It sounds like you already are planning some of this, but it would be
>> nice to cleanup a lot of the assumptions/hardcoded stuff/weirdness
>> while making these fairly intrusive changes. For example:
>>
>> - The relationship between documents and notebooks they're in, as you
>> discussed. As Lex mentioned, it would be nice to not make too much new
>> hard-coded assumptions about other documents only being allowed in
>> another notebook, but rather making it extensible to support multiple
>> windows in the future. Also as I mentioned, it would be nice to not
>> make any hard-coded assumptions about only having two split notebooks[1].
>
> The notebook of a doc can be retrieved via
> gtk_widget_get_parent(doc->editor->sci) without hardcoding. I'm using
> this where possible.
>

You're still hardcoding, just a different assumption as mentioned above.

>> - The relationship between documents (models) and Scintilla (views),
>> they should be almost completely independent. There shouldn't need to
>> have document->editor->scintilla, the document needn't care what view
>> it's in, only the reverse. I have no idea where GeanyEditor fits into
>> this, I've never understood why it exists; it's not a view, it's not a
>> model, and it's not really a controller either, it's like part wrapper
>> over Scintilla, part extension of GeanyDocument or something like this
>> I guess?
>
> Sorry, I think this item is out of scope for this work. FWIW, I don't
> quite understand the deal about GeanyEditor either.
>

It's not out of scope, but it is way more work than I think you're 
personally prepared to do. It's basically the reason no one fixed the 
split window before or worked to moving it into core, because it was too 
much work with the current way it's coded. If this kind of stuff gets 
cleaned up first, the split window thing becomes trivial (as well as 
multi-window, etc). I would've fixed up split window myself 10x by now 
if I didn't morally have to cleanup all this code to do it correctly.

>> - The lifetimes of documents. I don't see any reason to recycle a
>> fairly small structure like GeanyDocument, especially since we
>> basically set it up and tear it down each time anyway. I doubt the
>> overhead of freeing an old GeanyDocument structure and allocating a
>> new one later is worth the contortions it causes in code and the
>> weirdness in the plugin API.
>
> Seems completely unrelated to splitwindow.
>

It's more part of the overall problems with the way the code currently 
is, but you're right, it's mostly unrelated and could be addressed 
separately.

>> - This follows with above, document_get_current() should *never*
>> return NULL. It makes absolutely no sense to me to allow having Geany
>> open without a document open. It'd be like having Firefox open with no
>> tabs/webpages open. Either it should open a blank untitled document
>> when the last one closes (this option exists already IIRC) or Geany
>> itself should just close (probably too annoying :) These last two
>> would get rid of weirdness like doc->is_valid, DOC_VALID(),
>> "documents" macro wrapping documents_array, foreach_document() macro
>> to iterate documents, etc.
>
> Also unrelated.
>

Also related to cleaning up the code properly first, but not directly 
maybe with the scope of changes you're proposing.

>> - Encapsulating the GeanyDocument so that plugins can't mess with
>> read-only members. For example, it makes no sense to allow plugins to
>> change doc->read_only, or doc->file_name (one of them actually does
>> this). It would be nice to make the API consistent here, like we have
>> document_set_text_changed() to mark the document as ditry/clean, but
>> there's no getter like document_get_text_changed(), which is
>> inconsistent and it allows plugins to seriously break Geany if they
>> aren't careful. This one is of course fairly off-topic and could be
>> attacked separately afterwards, I just thought it was worth mentioning
>> since you talked about needing to break the plugin API, it might be
>> useful to improve it during the breakage.
>>
>
> Also unrelated. I agree that big API breakage should be as rare and
> concentrated as possible (i.e. within a single release) but not as part
> of a single change. Let's not blow this splitwindow work up with
> unrelated changes. This would just make review impossible and cost more
> time to get done.
>

The fact that it "blows up" into more work is exactly why we should bite 
bullet instead of keep "just making it work with minimal changes", when 
the code is factored well, making changes becomes easier. But I digress 
somewhat (as usual).

I withdraw my request to help out with this because I don't think you're 
interested in doing some of the bigger underlying changes I was 
interested in doing before attempting to fixup split window, and I don't 
want to drag you down. As long as you keep in mind the stuff mentioned, 
and don't make any additional same type of assumptions that were made 
and evolved previously, I think it won't be such a big deal to do the 
stuff I was talking about underneath your changes as a separate 
initiative later on.

>
>> I actually use the current crippled split view *extensively* so I
>> would also be really interested in helping out with this. If it was
>> useful we could make a branch on the main repository to work from and
>> get more visibility with it probably.
>>
>
> Fine with me. You can also make PRs on my github until this is set up
> (but beware that I still amend and rebase) :)
>

I can still make a branch for this but as mentioned above I won't be 
contributing much to it as it stands. I will be a good tester though 
because I *really* want a fully functional split window :)

>
>> Cheers,
>> Matthew Brush
>
> Thanks for your input.
>

Sorry to be such a downer, I just really hate seeing this vicious circle 
of adding more and more code ontop of other code that needs to be 
refactored, making everything all intertwined and hard to hack on.

Cheers,
Matthew Brush