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

Update to HTML Tidy 5.3.1 #4

Closed
wants to merge 32 commits into from
Closed

Conversation

geoffmcl
Copy link

Not sure if you are interested, since it appears you have not worked on this repo is quite a while...

But as the result of an issue filed on tidy I have upgraded my Notepad++ to use Tidy 5.3.1, in my fork and created a release zip...

@bruderstein
Copy link
Owner

Hi, and apologies for the much delayed response to this - there's lots of improvements here!

You're right, I don't really maintain this any more, but it'd be really nice to get an updated version out. Especially if we can add 64bit support via the CMake build.

How about we get this merged, then if you're interested, you can have access to this repo (or we'll move the main repo to you), and you can continue and update the releases in the plugin manager admin. How does that sound? If you don't want to, that's fine, just a thought :)

There's a couple of minor things I'd like to clean up before we merge this:

  • can you remove the binaries and zips from the repo - ideally with a rebase so the commits aren't there in the history (that's what github releases are for, and it means the repo history isn't huge with different versions of binaries)
  • can you remove the "convenient source find" - this doesn't seem to have any relevance here, and everyone has their own favourite grep replacement.

Thanks again for doing this, and really sorry I've neglected to answer it for so long!

@geoffmcl
Copy link
Author

geoffmcl commented Feb 7, 2017

@bruderstein no real problem about the delay in your response, except I have more or less forgotten what I did here ;=))

Concerning the the minor things, no problem about removing the zip, except again I need to work on how to remove it, and erase it from git history... just have not done that very often... any git clues welcome...

So will take me a few days to remind myself about it... especially how to now update the tidy submodule, again something I have not done that often... it is the only project where I experimented with such a submodule approach... but seems workable...

I would forget about the tiny fi.bat which uses my personal fa4 grep tool... it may give other developers an idea how to get with the code, just changing the exe name... or adding their own tool...

So yes, I am all for merging my fork back to yours, and would be pleased to have access to your repo...

I too do not particularly want my fork to become the main NppTidy2 repo, as I too will not have much time to devote to it... but would look at it now and then, at least trying to keep your repo up to the latest tidy...

While I use npp all the time, I do not use the tidy2 plugin, except for these tests...

So, good to hear from you... where to next?

Regards, Geoff.

@bruderstein
Copy link
Owner

To remove the commit with the binaries

git rebase -i f33f282

which will open an editor with all of your commits in. (Make sure you've fetched from this repo first)
You can then just delete the line with the 2 commits that add binaries, save and exit the editor, and the history will be rewritten without those commits.

I'd really rather you did remove the fi.bat - it really doesn't have anything to do with this repo, and many advanced grep tools don't need such a batch file, and everyone has their favourite grep replacement for doing such things. Let's limit the amount of things someone has to look through to find how to build etc.

@geoffmcl
Copy link
Author

@bruderstein ok after rebasing to your last, doing the git rebase -i f33f282, and then git push --force seems to have successfully remove the binary and the fi.bat...

Are we good to go?

@chcg
Copy link
Contributor

chcg commented Dec 7, 2017

@bruderstein Any new thoughts on this Dave?

  • Would be nicer to have the checkins squashed to some bigger building blocks.
  • The option\need to have an additional external tidy dir is not so clear to me.
  • problaby it should be possible to just have on build_me.bat with parameters (dafault: x86) to build both x86 and x64

@geoffmcl Doing the changes directly at the master of your fork repo is not ideal, if you wan't to resync with this upstream repo once your PR is merged. Better would have been to create a branch from master.

@geoffmcl
Copy link
Author

geoffmcl commented Dec 8, 2017

@chcg wow, the first comment here in about 10 months, almost 1.5 years since my first post here on Jun 15, 2016... somehow I feel @bruderstein has more or less abandoned this repo, sadly...

Would be nicer to have the checkins squashed to some bigger building blocks.

Well, the checkins are the history, but yes they can be squashed, and in doing the merge there is an option to squash and merge, which does this I think... merges all as 1 commit...

The option\need to have an additional external tidy dir is not so clear to me.

Not sure what is unclear to you?

In simple terms it was an option to use the tidy-html5/CMakeLists.txt, after the submodule had been downloaded, as opposed to having the tidy built from the root CMakeLists.txt, which has the big problem that that build is now out-of-date ... wrong library name, maybe the current tidy source list has changed, etc, etc, so I think this would fail... not tried...

In general I am quite unclear, unfamiliar, unknowning, ... about how to use git submodules, and always use the other option provided that has tidy built and installed entirely separately...

So I would have no problem removing this out-of-date options, and in fact removing the use of git submodules completely...

problaby (sic) ... possible ... build_me.bat with parameters (dafault: x86) to build both x86 and x64

Yes, for sure that is possible, but for developers, maintainers, like me is very inconvenient... I want, need, each build separate, since they all share the one name Tidy2.dll...

And so far I have a MSVC14 32 and 64 bit, and intend adding 2 MSVC10 builds, each of which will be packaged into a appropriately named ZIP, together with checksums... see release Tidy 5.6.0 as an example...

So maybe I will consider adding a build-vers.bat that not only supported build x86 or x64, but also the version of MSVC to be used... as a convenience for users... will think about this...

... Better would have been to create a branch from master.

Yes, now a year and a half older, with more github experience, this is what I should have done, for sure... but at this time this is water-under-the-bridge, and not sure how to backup to this... any ideas welcome...

As I work towards creating a new Tidy2.dll release, as time permits, have already found another fix required to do the MSVC 10 build, and ...

I still want a way the version number is generated from cmake rather than having to manually modify source files... maybe something about how cmake processed the .rc -> .res... this should be possible...

As you can see, still interested in providing appropriate Tidy2.dll's for NPP, packaged in ZIPs... seems a very good use of library tidy, and a great plugin for NPP...

Thanks for the feedback... look forward to more timely comments... thanks...

@chcg
Copy link
Contributor

chcg commented Dec 8, 2017

  • Regarding git submodule is something like svn externals, if you know svn. More or less a link to another repo which you could change quite easily to different branch, tag, revision via the normal switching of revisions of git in that directory. Not so much magic as it might look like. The initial setup is the most complicated step from my point of view.

  • Regarding version generating, see:

https://stackoverflow.com/questions/8887117/can-cmake-generate-configure-file

cmake can populate e.g. some header file with additional version information and I believe it is possible from within cmake to parse a version.txt file to get the major.minor version before the configure_file() step.

Or from git tags:
https://brianmilco.blogspot.de/2012/11/cmake-automatically-use-git-tags-as.html

@geoffmcl
Copy link
Author

geoffmcl commented Dec 9, 2017

@chcg, really thank you for the quick feedback...

Regarding git submodule is something like svn externals, ...

Well, if you look me up a little, you will see I am an old codger that started in computers before we had this wonderful sharing thing like free repositories...

To try to cut a very long story short, I cut my repo teeth on cvs, migrated to svn, dabbled in hg, and then to git, which I love, and undertand a little, and now use every day... over many projects...

But the few times I have dealt with git submodules, it never seems to be exactly what I expect!

I can see the idea, but the breaking point for me was that it seems it only pulls a specific version/tab/commit, unless and until the maintainer does an update...

Now, I can see how that is great for some projects! It gets a specific, known, working, source, until the maintainer has the time to do a test, and update...

But to me, for a project like this Tidy2.dll, that is all too slow... like at present a --recursive NppTidy2 fork clone gives me tidy 5.3.1, 2016.04.16... YUK! You think you are getting the latest source, but you are not unless you know, and do some extra steps...

When I build tidy2.dll I want it always linking with the very latest tidy... at least to benefit from the many fixes over the last more than a year...

So, in brief, I find submodules the wrong approach here... that's all... not that it does not work, if you get into learning its ways...

Now maybe that is because I am a current maintainer of tidy, so always have many tidy versions locally installed, using various build tools, and offer various release ZIP downloads, so the user does not really need to get into also building tidy from source...

From what I see of this plugin, and how it uses the tidy API, it can benefit from always using the latest...

And I will not get into the createDefaultConfig in Tidy2 source - frozen in time... and modern tidy has an new API that could generate a default config... but that seems a whole other topic...

If I wanted, had the time, to go the whole distance, I would make Tidy2.dll use tidy.dll. That would mean it could benefit from just replacing the tidy.dll with the latest... But that approach means more work, and understanding, from the user...

Given that NPP installs, by default, in an area that ordinary users are not used to messing with, I resist this urge, and link Tidy2.dll only with static tidys.lib, so they only have one file, Tidy2.dll to copy to the correct place... well still maybe a few files, if you include the quickref.html, and some config samples...

Maybe my user assumptions are wrong here, and am willing to listen to discussion...

Regarding version generating...

Yes, using a cmake generated header file, or even the .rc file itself, is certainly one way the already cmake generated version from a version file, like version.txt, can get into a .rc file... this I will certainly explore... it is really only so the About dialog contains the latest build information...

But I certainly do not want this to include a git hash reference, which is also possible with say git_describe, etc... I have seen this in some projects but find it ugly and quite meaningless to most users... At one time tidy included such a hash, but changed that to a more meaningful human readable version and date...

So, yes, I have a way forward for this... just the interest and time to do it...

And you did not touch on the need for multiple versions, aside from the simple 32 vs 64-bits... again maybe a whole different discussion... and the other topics touched... but no problem...

Anyway, as stated, thanks for the quick feedback...

@geoffmcl
Copy link
Author

@chcg as you may have noted, see Issue 4, have solved my setting Tidy2.dll About dialog versions, at least to my satisfaction for now... still to work on presenting multiple release binaries, as time and interest permits...

I should probably close this PR, and present one against my set-vers branch, and assume such a new PR would now include all my npptidy2 updates... but since there is no feedback from @bruderstein there seems little point at this time... oh, well... no problem...

@geoffmcl geoffmcl closed this Dec 15, 2017
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.

None yet

3 participants