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

Move filetypes.* and *.tags to separate directories #485

Merged
merged 2 commits into from Mar 19, 2016
Merged

Conversation

techee
Copy link
Member

@techee techee commented May 5, 2015

This patch is a reaction to the discussion here #479 - the filetype settings should be easier to discover by users and I think adding them to the config file menu helps. Also, it simplifies the process of customising per-filetype settings because it's not necessary any more to manually copy the config file.

On the way I couldn't resist the temptation and tried to make the data directory less messy. The filetype, tags and the remaining config files are mixed there - I moved the filetype config under the filedefs directory and tags files under tags (we use the same directory names inside .config). I did the same also with the installed data under $PREFIX/share/geany so all three directories (source tree, installed data and config dir) have the same structure.

@codebrainz
Copy link
Member

I like the idea, but I wonder how much documentation (manual, wiki, blogs, etc.) that moving the files around invalidates?

@techee
Copy link
Member Author

techee commented May 5, 2015

Yeah, I plan to update the documentation but since it's the boring part, I will only do it if the patches are considered OK (all of them are completely independent of each other - the data directory reorganisation isn't necessary for the filetype settings in the tools menu).

@elextr
Copy link
Member

elextr commented May 5, 2015

LGTM when documented.

@codebrainz
Copy link
Member

I don't think the manual will be too much trouble, but more stuff like the wiki, blogs, tutorials, stack overflow questions, etc that can't easily be changed by us. The other issue is confusion with the old files if they're left straggling. I assume decent package management systems would remove the old files in favour of the new ones (hopefully nobody customized the files in $prefix- even though I know some people do wrongly do that), but with Autotools, if you just do the usual make install, it will leave the old files in place and the put the ones too. Even if you did make uninstall with the changed build system I don't think it would remove the old files.

For the patch to make editing the files easier by menus, I think are great. For the ones that move around the filedefs, I'm overall in favour of these changes for being nicer/cleaner[0], but it might cause some grief/confusion for some users.

[0]: I even thought of doing this myself before, but never bothered due to the disruptions mentioned.

@elextr
Copy link
Member

elextr commented May 5, 2015

I don't think the manual will be too much trouble, but more stuff like the wiki, blogs, tutorials, stack overflow questions, etc that can't easily be changed by us.

Well, with the menu the dir layout becomes our internal implementation detail, so since we can't change those things to say "use the menu", they will just have to bitrot. Actually moving stuff so they are wrong will make people ask and get told.

Even if you did make uninstall with the changed build system I don't think it would remove the old files.

You have to make uninstall with the old system and then git pull and make install with the new one, or have two separate clones ;-)

Hopefully package systems will handle the problem since the package knows what it has installed.

@techee
Copy link
Member Author

techee commented May 5, 2015

One of the possibilities is also moving the files in the source tree only and leaving the install locations where they are.

@codebrainz
Copy link
Member

One idea to CYA would be to separate out the moving around of files from this PR, merge the rest, and then ask on the devel mailing list if any packagers will have grief with this (and of course @b4n who does the source release :)

@elextr
Copy link
Member

elextr commented May 6, 2015

On the re-arranging of files:

As confirmed on IRC by one of our packagers, there is no problem for users of packaged Geany, so its only the build from source brigade that has to delete the old stuff. "Build from source" people are expected to be more careful, so, providing we are told to do it, its up to us to delete the old files.

Its three separate commits, so in the worst case if some massive problem happens (like MY build breaks :) it can be separately un-done if needed.

@techee
Copy link
Member Author

techee commented May 6, 2015

There should't be any things like "my build breaks" - the only problem will be that if new geany is installed over the old one, there will be duplicate config files and the old config files will be more visible because they aren't in a subdirectory so users will probably grab those when copying them under .config. On the other hand, there shouldn't be the need to copy them manually any more because of the menu entries.

I can split each of the the "moving" patches into "move in the source tree" and "move in the installed data dir" because it's probably only the second one which might cause troubles.

@elextr
Copy link
Member

elextr commented May 6, 2015

@techee yeah, separating the patch for the install dir is probably a good way to go, though I agree with you that there isn't much risk anyway.

@b4n
Copy link
Member

b4n commented May 6, 2015

I agree with @codebrainz, it's nice but might cause annoyance with some people having done less-than-optimal things.

@b4n
Copy link
Member

b4n commented May 6, 2015

I can split each of the the "moving" patches into "move in the source tree" and "move in the installed data dir" because it's probably only the second one which might cause troubles.

it won't be as simple as that with AT, as it's doing everything for us :)
well, it's not hard, but you'll have to split $(filetypes) out of nobase_dist_pkgdata_DATA and put it in dist_pkgdata_DATA.

@b4n
Copy link
Member

b4n commented May 6, 2015

BTW, file moves look good, assuming you didn't miss any path update in the code.

@techee
Copy link
Member Author

techee commented May 6, 2015

BTW, file moves look good, assuming you didn't miss any path update in the code.

Exactly ;-).

Before I start fighting autotools and splitting the moving patches, is it actually needed or is the moving part OK as it is? It probably doesn't make sense to do it if the move happens in both directories.

@b4n
Copy link
Member

b4n commented May 6, 2015

Before I start fighting autotools and splitting the moving patches, is it actually needed or is the moving part OK as it is? It probably doesn't make sense to do it if the move happens in both directories.

Well, we'd have to take a decision whether moving the installed files is OK or not. I think it mostly is, but I might like a cycle to let people use it a bit -- and also test the path changes maybe :)
So I guess the easiest for me would be to keep the change as is, but hold them for this release (if we really do release "soonish") -- especially as this change is mostly cosmetic.

@techee
Copy link
Member Author

techee commented May 6, 2015

@b4n Makes sense, let's delay this until after the release.

What about the menu part? I think some users might find it useful and shouldn't cause any stability problems (hopefully). I could make it a separate pull request.

@b4n
Copy link
Member

b4n commented May 6, 2015

sure, the menu's good

@techee techee changed the title Add filetype configuration to Tools->Configuration Files for all filetypes Move filetypes.* and *.tags to separate directories May 11, 2015
@b4n b4n added this to the 1.26 milestone Jul 1, 2015
@techee
Copy link
Member Author

techee commented Jul 14, 2015

I've dropped the already merged filetype configuration menu entries and rebased the patch on top of master to resolve conflicts.

@b4n
Copy link
Member

b4n commented Nov 1, 2015

About compat, how hard/sensible would it be to try the old location if not found in the new one? e.g. "merge" the directories when reading them.

@b4n b4n modified the milestones: 1.27, 1.26 Nov 1, 2015
@techee
Copy link
Member Author

techee commented Nov 4, 2015

@b4n I guess that would be possible but I just can't imagine the situation where this would be needed. If someone installs the new geany version then the config files should be at the new locations (and if there's some problem e.g. during packaging then IMO it's better if things fail loudly and the package gets fixed instead of working around the problem in the code).

@b4n
Copy link
Member

b4n commented Nov 6, 2015

(I thought I posted a comment about that, but looks like I forgot)

@techee I was thinking about a misbehaving user that would have put some custom filetypes in teh system directory. However, we could very well accept that just like changes to existing files would get overwritten, new files can get ignored.

@elextr
Copy link
Member

elextr commented Nov 6, 2015

@b4n, adding custom filetypes to the system directory is not "misbehaving", its just installing the filetype for all users of the machine 😄

But if the administrator is smart enough to do that then they are smart enough to fix it after we move the files.

@techee
Copy link
Member Author

techee commented Nov 8, 2015

@b4n OK, I think I misunderstood your original post, I thought you meant reading new first, old second (and it's the other way round).

I think exceptions like this make things more confusing for users at the end. And after the patch things will work the same way as before for the "misbehaving" users - their updated filetypes won't be read (previously they were overwritten during the installation, after the patch they will be read from a different location). The new location should be mentioned in the release notes so people who need to update file definitions in the system directory know the new location.

@techee
Copy link
Member Author

techee commented Nov 8, 2015

Yeah and by the way using the old->new order would mean that the old filetype definitions could be read forever (unless the user explicitly deletes them) and the updated definitions in new Geany releases would be ignored.

@techee
Copy link
Member Author

techee commented Feb 11, 2016

@b4n Another "zombie". If you find changing the install location too risky, I can change the patch to update just the source tree data directory and keep the install location as it is.

@b4n
Copy link
Member

b4n commented Feb 28, 2016

@techee hum… could you instead hit me on head right after the release so we really get a cycle to try that? looks good to me like that, but as said earlier it might have corner breakage we might not see straight ahead. And it's not a visible or highly important change, regardless of it being nice, so I think I'd rather acknowledge the fact I should have merged this earlier yet merge it in 2 weeks.

ah, and apparently there are conflicts to fix.

@b4n b4n self-assigned this Feb 28, 2016
@b4n b4n modified the milestones: 1.28, 1.27 Feb 28, 2016
@techee
Copy link
Member Author

techee commented Feb 28, 2016

@b4n No problem, will ping you at the beginning of the next release cycle (will fix the merge conflicts then in case something else introduces more conflicts in the meantime).

@techee
Copy link
Member Author

techee commented Mar 13, 2016

@b4n Updated on top of master. This is a friendly reminder it's already 2 hours past release and time to merge this pull request :-).

@b4n b4n merged commit 3640b3b into geany:master Mar 19, 2016
b4n added a commit that referenced this pull request Mar 19, 2016
Move filetypes.* and *.tags to separate directories
eht16 added a commit that referenced this pull request Mar 21, 2016
This adapts the paths in the installer for the tags and filedefs files
as changed in #485.
Also simply include everything in share/ when including a GTK3
runtime environment instead of a fixed list of subdirectories,
share/glib-2.0 doesn't exist anymore when using MSYS2 packages.
@Skif-off
Copy link
Contributor

Hi,
file filetypes.common was also moved to filedefs, but "This file defines some general non-filetype-specific settings". Is it right?

@b4n
Copy link
Member

b4n commented Mar 21, 2016

@Skif-off that matches the hierarchy under the user config directory, so I'd say yes.

@elextr
Copy link
Member

elextr commented Mar 21, 2016

@Skif-off you need to account for the fact that not all contributors have English as a first language. It means that filetypes.common defines the default values for all filetypes, the "common" settings as it name says. As such keeping it with the other filetypes makes sense.

eht16 added a commit that referenced this pull request Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants