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

Some features have been implemented #105

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

Conversation

GILGAMESHTHEKING
Copy link

Implemented features:

  • Ability to pin a file
  • Search and tail entire folder
  • MaterialDesign based file open
  • MaterialDesign based Yes/No message dialog

@RolandPheasant
Copy link
Owner

Wow that is a huge PR. I will be able to take a good look at it in the next few days.

Thanks for the effort.

@magyargergo
Copy link

magyargergo commented Apr 21, 2016

Dear Roland,

Could you look at this? Do you have any comment about it? Would you summarize your thoughts? We have deadline. :(

Thanks,
Gergo

@RolandPheasant
Copy link
Owner

Sorry for the delay but I have been thinking how I can best respond as the PR is very large and complicated. Although there is some good code there are also some errors and poor choices.

I really appreciate the effort made to improve the system but in it's current state I cannot accept the code. I will explain below.

Cannot be automatically merged

It would have been better before submitting the PR to merge the latest master into your branch and resolving any merge conflicts first.

*Too many changes for a single PR *

By lumping together all the changes in one PR it means that I am forced to accept all or nothing. With several smaller PRs it would be possible to accept and reject on a case by case basis.

Pin a file

The way the pinning is implemented is not what I expected. My expectation was that to pin a file would mean to persist the file between sessions i.e. the next time TailBlazer is launched the file or files would be reloaded with the same state as last session.

Also as I stated in my email to you

Although you have chosen the features below, 'pin file' and 'pipe filter' are probably obsolete as they are currently being superseded by other ideas

The implementation in master now preserves the state of all open files between sessions so the idea of a pin in obsolete and cannot be accepted into TailBlazer.

Material Design File Chooser

The file chooser is very rough and would need a lot of work on it to make it a serious alternative to the standard file chooser. Also I had an unhandled exception.

However, I would like to take it and apply polish myself when I get a chance.

Tailing a folder

In the first instance, this does not work! I can open a folder and I cannot even scroll the result. The idea behind tailing a folder (or selection of files) was to enable treating rolled over log files as a single file. The explanation was given as:

'Tail entire folder' is the ability to look at multiple files and treat the result as a single file. For example traditionally when systems log to file, the file will roll-over when it reaches a certain size or will roll-over each new day. This means in time there are many different files for the same system. What I mean by tailing an entire folder would be enable those files to be 'virtually' joined together on the screen i.e. the most recent file is tailed and when the user scrolls, the scrolled lines of data scroll the files as if they were one. Has that explanation helped?

To allow for this concept the architecture has to become file agnostic. The only thing each screen should be aware of is that lines of data are provided. These lines could come from a folder, a multiple selection of files, or somewhere external like a unix ssh command output or even from Elastic Search output. If the current architecture is adapted correctly it would lead to the capability of plugging any of these in.

All of these concepts can be implemented being the following interface which I had very carefully designed.

    public interface ILineProvider
    {
        int Count { get; }
        IEnumerable<Line> ReadLines(ScrollRequest scroll);
    }

This interface would enable all the use cases above. However the proposed code has changed the simple and perfect interface into the following:

    public interface ILineProvider
    {
        int Count { get; }
        ILineProvider Previous { get; }
        ILineProvider Next { get; set; }
        int NumberOfPreviousProvider { get; }
        IEnumerable<Line> ReadLines(ScrollRequest scroll);
    }

These extra methods are unnecessarily placed on the interface. It is the implementation which should deal with details like reading the lines of the file. The simpler interface is applicable to all the uses case I mentioned whereas the extra methods add a burden to any further implementations.

Again I cannot accept this.

@magyargergo
Copy link

Could you explain the good part of the PR. I just read about the bad things.

@RolandPheasant
Copy link
Owner

Good points:

  • There has been a huge effort.
  • Excellent understanding of the file reading and filtering algorithms
  • Good implementation of multi FileWatcher
  • Some great understanding of Rx
  • Some great understanding of Wpf
  • The concept behind how the multi-file code has been implemented needs re-working but 80% of the actual code is right.
  • The file open dialog only requires some polish and it will be great
  • Lots of unit tests

I would love to take some of the code and re-organise it and apply polish as a lot of it is very beneficial

@alexanderfast
Copy link
Contributor

@GILGAMESHTHEKING I see some stuff in here I really like. Any plans on splitting up this pull request into many small ones (per feature)? I think that would be the only way forward.

@Noctis-
Copy link

Noctis- commented Feb 4, 2019

So ... this is dead ... ? :(

@raskolnikoov
Copy link

Any update when the feature "Search and tail entire folder" will be available?

@RolandPheasant
Copy link
Owner

I am been very busy for a long time on other open source commitments. However I intend to do a major revamp when dotnet core 3 is released. I will do it as part of that

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.

7 participants