Skip to content

Deprioritize git-ignored files#705

Merged
cassidyjames merged 16 commits into
elementary:masterfrom
seanballais:feature-653
Nov 25, 2019
Merged

Deprioritize git-ignored files#705
cassidyjames merged 16 commits into
elementary:masterfrom
seanballais:feature-653

Conversation

@seanballais
Copy link
Copy Markdown
Contributor

@seanballais seanballais commented Aug 31, 2019

Currently working on addressing #653. The image below shows my current progress.

PR Output

I still have not worked on supporting glob patterns inside .gitignore files. It'll be up as soon as I am able to work on this again.

Glob patterns inside .gitignore files are now supported. However, skipping certain files from being ignored (as done via !) is not yet fully supported. Deprioritizing files inside an ignored folder is not yet done.

I have added the feature that deprioritizes files. Changes to .gitignore automatically gets mirrored in the sidebar now. It still needs additional testing with folders and multiple project folders.

I also need an opinion from the UI team with regards what should the colour be for the text of the deprioritized files. At the moment, I have set the text colour to #BFBFBF. Instead of changing the text colour of the deprioritized files to #BFBFBF, I modified their opacity instead and set it to 50%. The decision to change the opacity instead of the text colour is based on the property of dim-label in the default elementaryOS stylesheet. Choosing dim-label as the basis for the aforementioned decision is due to @lainsce's suggestion of using Gtk.STYLE_CLASS_DIM_LABEL, which is the GTK enum constant of the dim-label CSS class. I originally set the opacity from 75%, as specified by dim-label, but reduced it to 50% since the text didn't look deprioritized enough.

@eyelash
Copy link
Copy Markdown
Contributor

eyelash commented Sep 1, 2019

Have you considered using Ggit.Repository.path_is_ignored for this?

@lainsce
Copy link
Copy Markdown
Contributor

lainsce commented Sep 1, 2019

I also need an opinion from the UI team with regards what should the colour be for the text of the deprioritized files. At the moment, I have set the text colour to #BFBFBF.

Instead of doing that, style the Gtk.Label with the Gtk.STYLE_CLASS_DIM_LABEL class, and remove it when the file is no longer in the gitignore file.
This ensures readability and contrast.

I can provide a code snippet if it helps you with this.

@seanballais
Copy link
Copy Markdown
Contributor Author

Have you considered using Ggit.Repository.path_is_ignored for this?

@eyelash, I haven't. I'll have a look at it once I am able to work on this again. Thanks!

Instead of doing that, style the Gtk.Label with the Gtk.STYLE_CLASS_DIM_LABEL class, and remove it when the file is no longer in the gitignore file. This ensures readability and contrast.

I can provide a code snippet if it helps you with this.

@lainsce, oh. I never knew about that class. I'm not sure if there is a Gtk.Label inside SourceList.Item, but I'll make use of that knowledge about that class. Providing me a code snippet on it would be helpful. Thanks! 😃

@cassidyjames
Copy link
Copy Markdown
Contributor

I'd also consider marking the item as insensitive instead of applying a class on the label itself, though that does probably imply that you wouldn't be able to edit that file. I guess I'm not sure if we'd want that or not.

@seanballais
Copy link
Copy Markdown
Contributor Author

seanballais commented Sep 7, 2019

I think it'd be best if we go for applying the class to the label instead of marking it as insensitive.

On another note, @cassidyjames, are we going to deprioritize files only if the project folder is a local git repository or are we still going to deprioritize if a .gitignore file exists? (I just realized that it doesn't make sense to have a .gitignore file when you're not using Git.)

@seanballais
Copy link
Copy Markdown
Contributor Author

In my latest commit, I used Ggit.Repository.path_is_ignored () in checking if a file or folder is git-ignored, as suggested by @eyelash. It works perfectly so far. I used it in the function is_file_gitignored () in ProjectFolderItem.

When catching any error path_is_ignored () throws, I would log the error message. However, I am unsure what log level to use. I noticed in the codebase that warning () is sometimes used, and sometimes critical (). But, I can only speculate the reasons why a warning () would be used instead of critical () and vice-versa. In the try-catch block I have in is_file_gitignored (), when logging an error message, I used a log level of WARNING since I don't feel that any error that would arise would deserve a log level of ERROR nor CRITICAL and I followed the log level that a similar try-catch block, the first such block in do_git_update () in ProjectFolderItem, used. However, I have a feeling that the log level should probably be ERROR. So, I'm not really confident that the log level should be WARNING.

What is the recommended log level to use in the case I presented? Is there a convention specific for elementaryOS codebases that details which log level to use?

@seanballais seanballais changed the title [WIP] Deprioritize git-ignored files Deprioritize git-ignored files Sep 13, 2019
@seanballais seanballais marked this pull request as ready for review September 13, 2019 16:05
item.activatable = null;

if (is_file_gitignored (item)) {
item.markup = Markup.printf_escaped ("<span fgalpha='50&#37;'>%s</span>", item.name);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels weird to have this repeated as well as in deprioritize_gitignored_files (), could we just re-use that method?

foreach (var child in top_level_item.children) {
var item = child as Item;
if (is_file_gitignored (item)) {
item.markup = Markup.printf_escaped ("<span fgalpha='50&#37;'>%s</span>", item.name);
Copy link
Copy Markdown
Contributor

@cassidyjames cassidyjames Nov 25, 2019

Choose a reason for hiding this comment

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

50% gets pretty inaccessible, so let's use 75% and italic:

Suggested change
item.markup = Markup.printf_escaped ("<span fgalpha='50&#37;'>%s</span>", item.name);
item.markup = Markup.printf_escaped ("<span fgalpha='75&#37;'><i>%s</i></span>", item.name);

image (1)

Copy link
Copy Markdown
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

See inline. Looks like you cannot use style classes as Item is an Object not a Widgets and does not offer styling functionality.

item.markup = Markup.printf_escaped ("<span fgalpha='50&#37;'>%s</span>", item.name);
} else {
item.markup = null;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Repeated code. Rather than repeat this code, call deprioritize_git_ignored_files () after resetting.


private void deprioritize_gitignored_files (Item top_level_item) {
foreach (var child in top_level_item.children) {
var item = child as Item;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Check for null child or child that is not an Item to avoid critical terminal messages when .gitignore is updated.

@cassidyjames cassidyjames merged commit 5c5cd1f into elementary:master Nov 25, 2019
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.

5 participants