Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect file changes #135

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Conversation

aaxu
Copy link
Contributor

@aaxu aaxu commented Jan 6, 2018

This PR includes a new object in the TextBuffer object that tracks the file on disk using threads. If in single-threaded mode, the object will not automatically track the file, but can be used to manually grab the information from disk.
So far, this thread sends requests to the background thread in background.py to notify the main thread to do things. So far, the thread will detect if the file is "Read Only", and if the file has changed since it was last saved/opened. If the file was changed since it was last saved/opened, a popup box will appear, asking the user if they would like to reload the file. For some reason, there is a bug somewhere which is causing the popup box to not appear the screen refreshes again, but I'm putting this PR up to just get some input for now, as I work on it some more.

I made the popup box so it made it easier to see for the user to see, and I figured we could also make other important messages appear in this box, like saving or overwriting a file.

app/window.py Outdated
self.host = host
self.controller = app.cu_editor.PopupController(self)
self.setTextBuffer(app.text_buffer.TextBuffer())
self.controller.setTextBuffer(self.textBuffer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider overriding setTextBuffer and set the controller.setTextBuffer there. It doesn't make a big difference right now, but maybe in the future: if if setTextBuffer is called it should tell the controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good.

#profile = app.profile.beginPythonProfile()
if message == 'quit':
app.log.info('bg received quit message')
return
elif message == 'popup':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the pop-up simply be a top level modal window in the ci_program.py zOrder, so that background.py wouldn't need to know anything about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by this? I'm making it so that the FileStats object contacts the background thread, and the background thread contacts the main thread. Right now background.py doesn't really know anything about the pop-up and it's just redirecting it to the main thread.

@aaxu aaxu force-pushed the enhancement/fileHasChanged branch from 3261263 to 216c5ac Compare January 11, 2018 17:35
@@ -0,0 +1,211 @@
import app.background
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a copyright comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@aaxu aaxu force-pushed the enhancement/fileHasChanged branch from 8a3f3bd to 362b8a3 Compare January 18, 2018 19:11
@dschuyler
Copy link
Collaborator

Sorry for the slow feedback (on my part) for this PR. I'll try to get more time with it this evening or Monday.

@aaxu
Copy link
Contributor Author

aaxu commented Jan 19, 2018

No worries! I've been taking a break from this PR because I just couldn't figure out the synchronization issue. Maybe I'll be able to give it another go this weekend.

Essentially the bug that is occurring is that sometimes, the popup window will not render on the screen, but the focus has changed to it. It will show up on the screen if any key is pressed (thus refreshing the screen and making it display). This could be troublesome because if the user types Y, they could accidentally reload the text file and lose all their current changes.

@dschuyler
Copy link
Collaborator

WDYT of separating the popup window to another PR (that would make reviewing it easier)

@aaxu
Copy link
Contributor Author

aaxu commented Jan 22, 2018

I could do that. It just seemed incomplete if I were to move the popup out of this PR and also I needed to test it using the popup, so I bundled them up together. I'll see if I can separate it out.

@aaxu aaxu force-pushed the enhancement/fileHasChanged branch from 362b8a3 to 8561c27 Compare January 22, 2018 19:30
aaxu added 21 commits January 27, 2018 15:37
…n file. Need to integrate this into the program
… refresh the TopInfo window, but doesn't seem to be displaying in the program
…nction in background.py. Also renamed refresh to redraw
…e program needs, which will prevent synchronization issues from occurring. Saving redo chain has now broken and needs to be fixed and still need to support fileStats when in singleThread mode
…in multithreaded mode. Also renamed getTrackedFileInfo to getUpdatedFileInfo to better reflect its functionality
…Stats object so that it will track the new file. Also created a FileTracker object that inherits from threading.Thread in order to speed up the killing of threads.
… which would check if the file on disk has changed
@aaxu aaxu force-pushed the enhancement/fileHasChanged branch from 8561c27 to 4ec9d8e Compare February 4, 2018 23:45
@aaxu
Copy link
Contributor Author

aaxu commented Mar 5, 2018

I've just updated this to work with the new master branch

@aaxu
Copy link
Contributor Author

aaxu commented Mar 14, 2018

@dschuyler Do you think there are any functionality changes that need to be made? It currently polls the file every ~2 seconds, but I don't think each poll has a lot of overhead. There's still that one synchronization bug, but I still have yet to figure out why it's happening.

@Androbin
Copy link

Androbin commented Mar 16, 2018

On Linux machines (and WSL), we can use inotify (native).
For Python, there are also various platform-independent libraries.

@aaxu
Copy link
Contributor Author

aaxu commented Mar 16, 2018

I looked into a different platform called watchdog which also used inotify. The issue with this is that when you do a git checkout, it doesn't modify the actual file, but I think it replaces it with another, so I couldn't see how to do it without watching the entire directory for changes.
Implementing it like this also reduces the number of dependencies and would be easier to make it better suit our needs, so this was what I opted for, at least for a proof of concept. However, maybe we can move on to a different platform for different host OS for better efficiency ( I don't know if macOS has its own kind of inotify).

@aaxu
Copy link
Contributor Author

aaxu commented Apr 12, 2018

@dschuyler are we moving away from using the Window.hide() function now? I see that there is now an assert False at the beginning of it.

@dschuyler
Copy link
Collaborator

Yes, Window.hide() is transitioning. I had hide/show removing or adding the window to the parent. At the moment, detach()/reattach() play a similar role. I'm up for suggestions on what to call these things. The concept I'm trying to name is one where:

  • the parent does not see the sub-view as a child. It doesn't pass events to it and such. It's "detached" from the parent.
  • the sub-window still knows who it's parent is suppose to be. The sub-window will still pass requests up to the parent (even though the parent doesn't see it as a child).
    I'm not 100% sure what the best answer is here yet, but that's why .hide() is in flux.

@aaxu
Copy link
Contributor Author

aaxu commented Apr 12, 2018

I actually have a question regarding the format of the program.

From what I understand, inputWindow is everything we see on the screen when we start up the program. This window contains a text buffer (which is where we display the file and type into the file), just like all other windows. However, this window also contains other windows like labeledLine, and lineNumbers.

Then, there is the programWindow which essentially contains the high level windows like inputWindows, palletteWindows, debugWindows, etc. I am guessing that when we decide to support split screen, we'll have multiple inputWindows? This programWindow also controls the zOrder and layering of all these high level windows. I don't think the inputWindows control or need to control any of its own windows.

Do I have the general idea correct?

@dschuyler
Copy link
Collaborator

Yes-ish.

Think of Windows being in a hierarchy. The top-most window (or root window) is a ProgramWindow window.

... inputWindow is everything we see on the screen when we start up the program.

Yes, though all of that is within the ProgramWindow, but the ProgramWindow doesn't have any UI of it's own. ProgramWindow could have UI, it just doesn't. It's used as a container to stick other windows into.

This window contains a text buffer (which is where we display the file and type into the file), just like all other windows. However, this window also contains other windows like labeledLine, and lineNumbers.

Yes.

Then, there is the programWindow which essentially contains the high level windows like inputWindows, palletteWindows, debugWindows, etc. I am guessing that when we decide to support split screen, we'll have multiple inputWindows?

Yeah, that's the idea.

This programWindow also controls the zOrder and layering of all these high level windows.

Yeah. Actually every window can have child windows. The zOrder is the list of child windows. I've considered changing the member variable zOrder to children. Whatever it's called, it is a list of child windows ordered by their 'depth' in the Z axis (where X and Y are horizontal and vertical on the screen). The (fake) Z depth is done using the painter's algorithm (which just means we draw the things furthest away first and overlay the nearer things later, like a painter does when painting a scene on a canvas).

I don't think the inputWindows control or need to control any of its own windows.

I don't understand what this is saying, sorry. Could you say or ask this in another way?

Do I have the general idea correct?

Sounds like it, though I'm not sure on that last one.

@dschuyler
Copy link
Collaborator

Trivia: Initially ci_edit didn't have a ProgramWindow. I was using the CiProgram as the topmost window. That got weirder and more cumbersome over time. Eventually I created ProgramWindow to give a cleaner top-level window and a cleaner separation between CiProgram and the window hierarchy. I'm glad I did. It's been helpful. So there should only every be one ProgramWindow and it mainly exists to keep the code organization cleaner.

@dschuyler
Copy link
Collaborator

dschuyler commented Apr 13, 2018

Bonus:

I printed out a hierarchy for ci_edit when it's first started and annotated what the pieces are:

<app.program_window.ProgramWindow -- root window, container for other windows
  <app.file_manager_window.FileManagerWindow -- the file path line
    <app.window.LabeledLine -- the blank line at the bottom
    <app.file_manager_window.DirectoryList -- the list of files
      <app.window.OptionsRow -- the header line in the list of files
    <app.window.OptionsRow -- the title line at the top
    <app.window.OptionsRow -- options like show dot files, etc.
  <app.window.InputWindow -- the main edit window
    <app.window.ViewWindow -- the upper left where it says "ci"
    <app.window.ViewWindow -- right hand column (one char wide)
    <app.window.LineNumbers -- left hand line number column
    <app.window.StatusLine -- line at the bottom that shows cursor row,col.
    <app.window.TopInfo -- top two lines that show context
    <app.window.InteractiveFind -- a container without its own UI
      <app.window.LabeledLine -- the "Find:" line
      <app.window.LabeledLine -- the "Replace:" line
      <app.window.RowWindow -- line with the options toggles
        <app.window.OptionsToggle -- "[x] regex"
        <app.window.OptionsToggle -- "[x] wholeWord"
        <app.window.OptionsToggle -- "[x] ignoreCase"
    <app.window.MessageLine -- line at very bottom (normally blank)

[edit: I'd mislabeled/omitted the right column view and TopInfo.]

@aaxu
Copy link
Contributor Author

aaxu commented Apr 13, 2018

Oh what I meant was just that I thought it would have been better to have all the painting and layering done by the programWindow and not the inputWindow as well, but now I'm thinking each window should control its own subwindows to make it easier (like what we have right now).

@aaxu
Copy link
Contributor Author

aaxu commented Apr 13, 2018

Also, what do you think about making some variables global, or adding a reference to the programWindow to all windows when initialized. I find that if I want to get access to the programWindow, or the popup window, I sometimes would have to do self.view.host.host or something. This makes it pretty confusing to keep track of what exactly you're referencing. If we end up going into a very deep subwindow, I could see a long list of self.view.host.host.host.....

@Androbin
Copy link

Androbin commented Apr 13, 2018

How about a global object windows mapping e.g. windows.edit_main to the app.window.InputWindow instance etc. using __getattr__ ?

@dschuyler
Copy link
Collaborator

I'd like to avoid globals (even though I use them at times). Can the desired effect be done with this paradigm

# In a base class, such as ViewWindow.
def doFoo(self):
  self.parent.doFoo()

# In some parent, such as ProgramWindow:
def doFoo(self):
  reallyDoTheFoo()

@dschuyler
Copy link
Collaborator

P.S. I also agree that self.view.host.host.host... is terrible. I intend to reduce that style.

@Androbin
Copy link

@dschuyler How about constructing windows dynamically from the current node?
This doesn't require a global, just the depth of the current node (the number of .hosts).

@Androbin
Copy link

For example: self.windows.edit_main.do_something() where windows is also a dynamic attribute.

@dschuyler
Copy link
Collaborator

How about constructing windows dynamically from the current node?

I'm not 100% sure I understand, sorry.

New note about parent vs host (trivia): I'd initially conflated parent and host and I should not have. A parent is the Window immediately above this window in the hierarchy. A host was intended to be the window that holds the current document. Often the host would equal the parent (so often that I conflated them), but the host could be several parents up.

Lately I've been thinking that host is not a good idea (at all). Instead it may be better to pass events (function calls) to self.parent.foo() until a parent designated to handle the situation handles it (i.g. that instance declares itself 'host' by virtue of handling the foo()) -- so the child window doesn't really know who the host is, but it can ask it's parent to pass up the message.

Goals: I'm looking for a solution that

  • is robust in the application and in unit tests (globals make problems in unit tests).
  • allows for changing the window hierarchy (having a child assume the host is N levels up is a problem for this)
  • allows having multiple document windows (globals are a problem here as well)
  • is a repeatable paradigm, i.e. the same way of doing it can apply in different situations (I want to avoid having different things do different paradigms).

What is in ci_edit right now does not reach those goals (my fault, it's something I'm working on fixing).

@dschuyler
Copy link
Collaborator

For example: self.windows.edit_main.do_something() where windows is also a dynamic attribute.

Thanks, I'll think on that more.

@aaxu
Copy link
Contributor Author

aaxu commented Apr 13, 2018

Would we have one inputWindow per file open / per view of a file open? I think this would make supporting multiple files easier since we wouldnt have to worry about switching out text buffers and resetting all the windows.

We can maybe store all open files as "inputWindow“s in memory. The program window can have a list of inputWindows to display, as well as a list of open but inactive (not being displayed) inputWindows (files/views). The program window can then have pre defined formatting for displaying 1 or 2 inputWindows, which can just be what we have right now, or split screen.

I also imagine having a "tabs" section (maybe a subwindow in programWindow) that shows open windows. Changing tabs would just mean swapping the programWindows displayed inputWindows with the clicked one.

Im thinking that we could make most windows recursively ask their parent for an action to occur, just as you mentioned before. This would solve the references for an ancestor N levels above.

@dschuyler
Copy link
Collaborator

Just some notes that will hopefully clarify the current intent.

For open files and InputWindows the intent is to allow for many open TextBuffers and many InputWindows.

Any given InputWindow will have exactly one TextBuffer. If there are no open TextBuffers from disk, an empty (new) TextBuffer is created for the InputWindow.

A TextBuffer can exist without an InputWindow. E.g. a TextBuffer can be loaded, manipulated, and written back to disk without ever appearing in an InputWindow (this doesn't currently happen, but the design is that it could be done).

The TextBuffer that an InputWindow is looking at can be changed (this happens every time a new file is viewed without quitting the editor, since there's only one InputWindow currently)*.

In the future, there may be many InputWindows. Any InputWindow may be viewing any of the TextBuffers.

Two or more InputWindows may be using the same TextBuffer. Edits made in either InputWindow will appear in the other.

If tabs are implemented*, the TabRowWindow (or whatever it may be called) would likely be a child of the ProgramWindow. The InputWindows may be either siblings of the TabRowWindow (makes sense if there is a single tab controller); or they may be children of a TabRowWindow (makes sense if there may be multiple tab controllers).

*The currently viewed TextBuffer within an InputWindow can be changed with ctrl+p.

@aaxu
Copy link
Contributor Author

aaxu commented Apr 16, 2018

Right, so for right now, should we look towards refactoring a bit to support this paradigm? If we are going to change the hierarchy and relationships between windows, it would be easier to do it earlier than later.

Maybe for now we can do something like the following:

  • Make windows only talk to their direct parent/children
  • Add functions that would allow windows to ask for the program window and inputwindow

This would make it easier to:

  • Finish this PR and make it function without making many assumptions about the hierarchy

Also one more thing. Should we add the inputWindow's textBuffer into its own window (FileContentWindow)? It seems like the text buffer object is just floating around in the inputWindow while being surrounded by other windows, though I don't know how necessary this new window would be.

@dschuyler
Copy link
Collaborator

Maybe for now we can do something like the following:
Make windows only talk to their direct parent/children

This is my current goal (though the code isn't there yet)

Add functions that would allow windows to ask for the program window and inputwindow

My hope is that passing messages up parent to parent will alleviate the need for asking for the program window or input window.

@Androbin
Copy link

Androbin commented Apr 17, 2018

Make windows only talk to their direct parent/children

So our paradigm is the Mediator Pattern, isn't it? The Polymer docs phrase it like this:

Polymer implements the mediator pattern, where a host element manages data flow between itself and its local DOM nodes.

When two elements are connected with a data binding, data changes can flow downward, from host to target, upward, from target to host, or both ways.

When two elements in the local DOM are bound to the same property data appears to flow from one element to the other, but this flow is mediated by the host. A change made by one element propagates up to the host, then the host propagates the change down to the second element.

@aaxu
Copy link
Contributor Author

aaxu commented Apr 21, 2018

Yes that's the paradigm I'm following. I'm trying to work a bit on refactoring this.
Is a view just the name for what an object calls the Window object that it is contained in? For example, textBuffer.view will always reference a window object.

@aaxu
Copy link
Contributor Author

aaxu commented Apr 21, 2018

I don't know if we can follow that exactly. The hierarchy looks something like this right now

                ProgramWindow
                      |
                 InputWindow
               /            \
    OtherWindows (tree)     textBuffer

Some windows need to reference the textBuffer object under InputWindow, and then need to make a function call. However, calling textBuffer.function() would violate the relationship paradigm since they don't have direct access to the buffer. We could make functions in inputWindow that call these functions, but then that would also need to propagate throughout the tree, and we would need a huge amount of extra functions. We could maybe make the textBuffer a shared resource throughout all windows under inputWindow, by passing it into the constructor of every window, which would allow us to avoid this.
How should we approach this?

@Androbin
Copy link

We could maybe make the textBuffer a shared resource throughout all windows under inputWindow, by passing it into the constructor of every window, which would allow us to avoid this.

Very good point indeed. Now that I think about it, we might want to have more than one tree...

View:       ProgramWindow          | Model:    SharedResources
                  |                |          /               \
             InputWindow           | OtherResources (tree)     TextBuffers
           /            \          |                          /     |     \
OtherWindows (tree)     textField  |               textBuffer  textBuffer  textBuffer
                       (ipsum.txt) |               (lorem.txt) (ipsum.txt) (dolor.txt)

@dschuyler
Copy link
Collaborator

Is a view just the name for what an object calls the Window object that it is contained in?

Yes. In some GUI frameworks words like Window, View, Facet, Surface, and Pane have distinct meaning. The ci_edit GUI hasn't progressed far enough to make distinct meanings. So at the moment they are rather interchangeable. This is a flaw, though; I we should define them.

For example, textBuffer.view will always reference a window object.

Yes (or it may be None).
A textBuffer may have zero to many views (there is a bug in the current code that a textBuffer expects to have zero or one view).
A window will have one textBuffer (not zero or more than one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants