GNOME Bugzilla – Bug 781542
Refactore StorageAnalyzer and separate it
Last modified: 2021-05-25 17:31:58 UTC
Clean up StorageAnalyzer code for better readability of source code and/or separate it into logical units. Also instead of using an absolute address to use URI everywhere.
I am working on it.
Created attachment 350305 [details] [review] Clean up StorageAnalyzer code and separate it for better readability.
Created attachment 350488 [details] [review] Remove commented code for build with autovala
Created attachment 351043 [details] [review] Clean up StorageAnalyzer code and separate it for better readability. Fix indentation - 4 spaces.
Created attachment 351466 [details] [review] Clean up StorageAnalyzer code and separate it for better readability.
Created attachment 351467 [details] [review] Clean up StorageAnalyzer code and separate it for better readability. https://bugzilla.gnome.org/show_bug.cgi?id=781542 Fix swap path for uri.
Created attachment 351560 [details] [review] storage: Separate StorageAnalyzer & use URI Clean up StorageAnalyzer code for better readability of source code and separate it into logical units. Also instead of using an absolute address to use URI everywhere.
Review of attachment 350488 [details] [review]: sure. lgtm
Review of attachment 351560 [details] [review]: Thanks for your patch! I have suggested turning some objects into singletons. This is not always the way to go because it hides the dependencies between the code. We should have a better object hierarchy with interfaces and abstract classes as much as possible. Other than that, try to make your classes follow the "single responsibility principle" of object oriented programming. ::: src/storage-actionbar.vala @@ +103,3 @@ chooser.set_filter(filter); StorageRow storage_row = (StorageRow) ((StorageView) (GLib.Application.get_default() as Application).get_window().get_views()[2]).get_storage_list_box().get_selected_row(); + chooser.set_filename(File.new_for_uri(storage_row.get_item_uri()).get_path()); If the row has a GFile, you don't need a get_item_uri method, and you can feed it directly to the filechooser. chooser.set_filename(storage_row.item.get_path()) ? @@ +112,3 @@ { StorageRow actual_storage_row = (StorageRow) row; + string destination_uri = chooser.get_file().get_uri() + "/" + Path.get_basename(actual_storage_row.get_item_uri()); Use GLib.build_filename* methods to assemble paths and uris, please. @@ +258,3 @@ { StorageRow storage_row = (StorageRow) row; + var file_operations = new StorageFileOperations(); new StorageFileOperations everywhere. ::: src/storage-analyzer.vala @@ +158,3 @@ + } + + use Vala's getters and setters instead? @@ +261,3 @@ + get_uri_for_path(Environment.get_user_special_dir(UserDirectory.PICTURES)), + get_uri_for_path(Environment.get_user_special_dir(UserDirectory.VIDEOS)), + get_uri_for_path(Environment.get_user_data_dir() + "/Trash/files") Glib.build_filename ::: src/storage-file-operations.vala @@ +13,3 @@ + var dest_parent = dest_file.get_parent(); + var type = src_file.query_file_type (FileQueryInfoFlags.NOFOLLOW_SYMLINKS); + Array<string> folders_for_delete_from_cache = new Array<string>(); folders_to_delete_* @@ +35,3 @@ + if(type == FileType.DIRECTORY) + { + for(int i = 0; i < folders_for_delete_from_cache.length; i++) foreach is more elegant. @@ +105,3 @@ + var file_info = file.query_info (FileAttribute.STANDARD_SIZE, FileQueryInfoFlags.NOFOLLOW_SYMLINKS); + size = file_info.get_size(); + } this conditional has been repeated a lot in this source code. You could write a new method for it instead of duplicating the if (type == FileType.DIRECTORY... ::: src/storage-graph.vala @@ +190,2 @@ var two_graphs = false; + if(storage_list_box.get_root() && (GLib.Application.get_default() as Application).get_storage_analyzer().separate_home_partition) Can you somehow refactor the class structure to avoid as much as possible getting properties from the GtkApplication instance everywhere in the code like this one above? The objects should have a more clear/organized hierarchy. ::: src/storage-item.vala @@ +32,1 @@ public class StorageItem : Object StorageItem could hold a whole GFile reference instead of having it's own size, path, uri bits. ::: src/storage-list-box.vala @@ +89,3 @@ loading(); + storage_analyzer.scan_cache_complete.connect(() => { + var storage_manager = new StorageManager(); Instead of creating a new instance of it for every object, you could make it a Singleton. ::: src/storage-manager.vala @@ +3,3 @@ +namespace Usage +{ + public class StorageManager : Object private static StorageManager storage_manager; public static StorageManager get_default () { if (storage_manager == null) storage_manager = new StorageManager (); return storage_manager; } This way you can use storage_manager = StorageManager.get_default () all over the code without creating multiple instances of it (Singleton). @@ +6,3 @@ + { + private StorageAnalyzer storage_analyzer; + private static bool root; is_root? @@ +56,3 @@ + + storage_analyzer.get_size_of_directory(StorageAnalyzer.get_uri_for_path(Environment.get_user_special_dir(UserDirectory.PICTURES))) + + storage_analyzer.get_size_of_directory(StorageAnalyzer.get_uri_for_path(Environment.get_user_special_dir(UserDirectory.VIDEOS))) + + storage_analyzer.get_size_of_directory(StorageAnalyzer.get_uri_for_path(Environment.get_user_data_dir() + "/Trash/files")); I have seen this elsewhere in the code. Could be static defined in a single place. @@ +63,3 @@ + } + + private void add_home_items(ref List<StorageItem> items, StorageAnalyzer.Storage storage, int section) could you iterate over these items instead of these IFs? @@ +335,3 @@ + } + + private static int sort_alphabetically(string first, string second) Make the object have a *compare ()* method instead. @@ +340,3 @@ + for(int i = 0; i < length; i++) + { + if(first[i] < second[i]) Do we know for sure that these strings are all ASCII? If you compare two CP932 strings using this function, you will get false matches. If this is desired, you can use first.ascii_casecmp (second) instead. @@ +351,3 @@ + { + root = true; + storage_analyzer = (GLib.Application.get_default() as Application).get_storage_analyzer(); Make the storage analyzer a singleton. This way, this line would be as simple as: storage_analyzer = StorageAnalyzer.get_default (); obs.: the "get_default ()" method name is widely used in our code base, so I recommend naming it this way. ::: src/storage-row.vala @@ +28,3 @@ box.margin_bottom = 12; var size_label = new Gtk.Label(Utils.format_size_values(storage_item.get_size())); + item_uri = storage_item.get_uri(); Hold a whole reference to StorageItem instead. It would be easy to handle uri changes in the go. @@ +168,3 @@ } + public string get_item_uri() Vala ships getters and setters for properties. @@ +269,3 @@ private void action_trash_restore() { Timeout.add(0, () => { 0 timeout? why? @@ +270,3 @@ { Timeout.add(0, () => { + var file_operations = new StorageFileOperations(); StorageFileOperations, the way you are using, could be a singleton as well. @@ +337,3 @@ box.margin = 10; var entry = new Gtk.Entry(); + entry.set_text (Path.get_basename(item_uri)); You could define the whole row Gtk+ specifics in a template file, to decouple it from the code logic. Later you can bind the GtkEntry "text" property with the Item uri property. This code I wrote does pretty much the same, but for a GtkFlowBoxChild https://git.gnome.org/browse/gnome-boxes/tree/src/icon-view-child.vala?h=wip/feborges/flowbox&id=cf83c8632192e1a1f002b03c97c51d662c6cc047#n69 In the linked line, specifically, is the binding bits I mentioned above. ::: src/storage-worker.vala @@ +11,3 @@ private AsyncQueue<StorageResult> results_queue; + private File file; + private string[] exclude_uris; Make it a list? @@ +24,3 @@ private uint64 get_directory(File file) { + string uri = file.get_uri(); If you are holding a reference to the File, there's no need to storage the uri in another property. What if it changes?
Review of attachment 350488 [details] [review]: Pushed as 59b152a4ddbc544d809fc31835e8699fac710ab8.
Created attachment 352492 [details] [review] storage: Separate StorageAnalyzer & use URI Clean up StorageAnalyzer code for better readability of source code and separate it into logical units. Also instead of using an absolute address to use URI everywhere.
Created attachment 352493 [details] [review] storage: Separate StorageAnalyzer & use URI Clean up StorageAnalyzer code for better readability of source code and separate it into logical units. Also instead of using an absolute address to use URI everywhere.
Created attachment 352514 [details] [review] process: Use properties & refactore process and monitors Replace variables for properties in process class. Refactore Process, CpuMonitor and MemoryMonitor classes. Replace tabs for 4 spaces.
Review of attachment 352514 [details] [review]: Please, split it in multiples commits. Make it module/component/class specific. Commits should be as atomic as possible. Every important change should be in a commit of its own. If you are listing/enumerating changes in your commit messages, such as "Refactore Process, CpuMonitor and MemoryMonitor classes. Replace tabs for 4 spaces.", you are probably doing at least 5 major changes, which should be 5 different commits (at least). For reference of the benefits of the practice: https://www.freshconsulting.com/atomic-commits/
Created attachment 352640 [details] [review] process: Use properties & refactore process and monitors Replace variables for properties in process class. Refactore Process, CpuMonitor and MemoryMonitor classes. Replace tabs for 4 spaces.
(In reply to Felipe Borges from comment #15 and Petr Štětka comment #16) Oh ok, I will split it to multiples commits.
Review of attachment 352640 [details] [review]: I will split it to multiples commits.
Created attachment 352643 [details] [review] system-monitor: Do SystemMonitor class singleton We have only one instance of SystemMonitor, so we can use singleton pattern for SystemMonitor class.
Created attachment 352646 [details] [review] monitor: CpuMonitor, MemoryMonitor must implement Monitor interface
Created attachment 352649 [details] [review] process: Replace getters and setter with properties Instead of use variables and getters and setters we can use properties. This change will alow easier coding for as.
Review of attachment 352493 [details] [review]: Lets split this one in two patches as well. The Use URI part could be another one. ::: src/header-bar.vala @@ +135,2 @@ storage_back_button.hide (); + StorageAnalyzer.get_default().create_cache.begin(true); Do you think we could somehow get this logic out of the header-bar? I would like to keep the header-bar to Gtk UI specific stuff. ::: src/storage-actionbar.vala @@ +98,3 @@ { StorageRow storage_row = (StorageRow) row; + if(filter_info.uri == storage_row.storage_item.file.get_uri()) storage_row could have a method to get URIs, since it is an UI representation of a file. I suggest storage_row.uri where uri { get { storage_item.file_get_uri (); } set; } @@ +133,3 @@ if(files != "") files += ", "; + files += storage_row.storage_item.name; same as above. It could be a property of storage_row.name... @@ +214,3 @@ + var file_operations = StorageFileOperations.get_default(); + file_operations.wipe_folder.begin(File.new_for_uri(storage_row.storage_item.file.get_uri()), () => { + ((StorageView) (GLib.Application.get_default() as Application).get_window().get_views()[2]).get_storage_list_box().refresh(); I am seeing this a lot ((StorageView) (GLib.Application.get_default() as Application).get_... Could we somehow have a shortcut or a hierarchy so that we don't need to access the main Application class all the time everywhere? ::: src/storage-item.vala @@ +271,3 @@ + return 1; + } + return first.length < second.length ? -1 : 1; What about non-ascii sorting? I think we addressed this before in another review. ::: src/storage-list-box.vala @@ +34,3 @@ private StorageItemType? actual_parent_type; private ListStore model; + private bool is_root = true; Make it a full property. While getting "get {}" you could query whether is_root is actually true. @@ +113,2 @@ var header_bar = (GLib.Application.get_default() as Application).get_window().get_header_bar(); + if(is_root == false) !is_root
I will get back to the rest of it later.
> Lets split this one in two patches as well. The Use URI part could be another one. Can we keep it in one commit? Both changes are very intertwined, and they are refactoring the majority of the code, and I think splitting the changes into two commmites would be a bit of extra work.
> storage_row could have a method to get URIs, since it is an UI representation of a file. > I suggest storage_row.uri where uri { get { storage_item.file_get_uri (); } set; } Do you mean? public string uri { get { return storage_item.file.get_uri (); } } However it will not work: error: Return value transfers ownership but method return type hasn't been declared to transfer ownership. So I used this "hack" I'm not sure there's a better way? public string uri { get { _uri = storage_item.file.get_uri (); return _uri; } } private string _uri;
Created attachment 356021 [details] [review] storage: Separate StorageAnalyzer & use URI Clean up StorageAnalyzer code for better readability of source code and separate it into logical units. Also instead of using an absolute address to use URI everywhere.
Review of attachment 356021 [details] [review]: Thanks for the patches! I am not finished yet with this patch, but you could rebase it based on my reviews so far. Lets make the following changes smaller and atomic. But refactors take as much time to review as it takes to write the code itself. ::: src/header-bar.vala @@ +118,3 @@ private void on_performance_search_button_toggled () { /* TODO: Implement a saner way of toggling this mode. */ + ((PerformanceView) get_views()[0]).set_search_mode(performance_search_button.active); Instead of having all these callbacks (one for each view), you could *bind* these properties in a single place. performance_search_button.bind_property ("active", performance_view.search_bar, "search-mode-enabled", BindingFlags.SYNC_CREATE); @@ +123,3 @@ [GtkCallback] private void on_storage_back_button_clicked () { + ((StorageView) get_views()[2]).get_storage_list_box().on_back_button_clicked(); Same. #bind @@ +274,3 @@ + private View[] get_views() + { + return (GLib.Application.get_default() as Application).get_window().get_views(); Wouldn't it be easier to have the views getting the HeaderBar and toggling the widgets sensitivity there instead of getting the views in the HeaderBar code? Just an opinion, I am ok if you want to keep it this way. ::: src/storage-actionbar.vala @@ +105,3 @@ chooser.set_filter(filter); + StorageRow storage_row = (StorageRow) get_storage_list_box().get_selected_row(); + chooser.set_filename(File.new_for_uri(storage_row.uri).get_path()); Make it chooser.set_filename (storage_row.path); @@ +114,3 @@ + StorageRow actual_storage_row = (StorageRow) row; + string destination_uri = Path.build_filename(chooser.get_file().get_uri(), Path.get_basename(actual_storage_row.uri)); + var file_operations = StorageFileOperations.get_default(); You are referencing the StorageFileOperations singleton in every iteration of the loop. Can we optimize it? @@ +115,3 @@ + string destination_uri = Path.build_filename(chooser.get_file().get_uri(), Path.get_basename(actual_storage_row.uri)); + var file_operations = StorageFileOperations.get_default(); + file_operations.move_file.begin(File.new_for_uri(actual_storage_row.uri), File.new_for_uri(destination_uri), () => { actual_storage_row.path @@ +133,3 @@ if(files != "") files += ", "; + files += storage_row.item_name; Make it a List and convert it to string (comma separated) only when you need it. @@ +145,3 @@ + { + StorageRow storage_row = (StorageRow) row; + var file_operations = StorageFileOperations.get_default(); Can this also be out of the loop? @@ +161,3 @@ { + foreach (Gtk.ListBoxRow row in get_storage_list_box().get_selected_rows()) + { I see that this block is repeating a lot. You could introduce your own foreach/forall method. StorageView.foreach_row (callback); @@ +178,3 @@ + { + StorageRow storage_row = (StorageRow) row; + if(storage_row.storage_item.item_type == StorageItemType.TRASH) storage_row.type StorageRow is encapsulating a single StorageItem, no need to expose it. @@ +181,2 @@ { + var dialog = new Gtk.MessageDialog ((GLib.Application.get_default() as Application).get_window(), Gtk.DialogFlags.MODAL, Do we want a MessageDialog? What about in-app notifications? See https://git.gnome.org/browse/gnome-boxes/tree/src/notification.vala ::: src/storage-file-operations.vala @@ +190,3 @@ + } + catch (Error e) { + stderr.printf ("Error: %s\n", e.message); debug("Failed to obtain folders list: %s\n", e.message); @@ +222,3 @@ + } + catch (Error e) { + stderr.printf ("Error: %s\n", e.message); ditto. ::: src/storage-row.vala @@ +25,3 @@ { + public StorageItem storage_item { get; private set; } + public string uri { If you also had a "path" property, you wouldn't have to create File.new_from_uri everywhere else.
Created attachment 358137 [details] [review] storage-analyzer: Make StorageAnalyzer class singleton I splitted patch to more commits and fix by your comments. Next changes can be another commits and patches I thing... as for ex. MessageDialog..
Created attachment 358138 [details] [review] color-rectangle: Use property & constructor
Created attachment 358139 [details] [review] storage: Refactore and rework storage Refactore and rework storage with async loading support.
Created attachment 358140 [details] [review] storage-row: Port to Gtk+ widget template
Created attachment 358141 [details] [review] storage-actionbar: Port to Gtk+ widget template complete
Review of attachment 352643 [details] [review]: looking good.
Review of attachment 352646 [details] [review]: nice!
Review of attachment 352649 [details] [review]: ::: src/process-row.vala @@ +46,3 @@ + load_label = new Gtk.Label(null); + load_label.ellipsize = Pango.EllipsizeMode.END; + load_label.max_width_chars = 30; try not to make unrelated changes in a commit. These seem to be just indentation changes, unrelated to the commit message. ::: src/sub-process-sub-row.vala @@ +66,1 @@ + if(process.cpu_load >= 90) Make 90 a constant so we can understand better what this line does. @@ +75,1 @@ + if((((double) process.mem_usage / monitor.ram_total) * 100) >= 90) Try to create utility functions for these type of logic so we can be more clear about what these type of lines does. ::: src/system-monitor.vala @@ +145,3 @@ foreach(unowned Process process in process_table.get_values()) { + if (process.alive == false) if (!process.alive) @@ +313,2 @@ { + unowned Process process = to_table[process_it.cmdline].sub_processes[process_it.pid]; these are too nested. Is there any refactoring we could do here?
Review of attachment 358137 [details] [review]: ok
Review of attachment 358138 [details] [review]:
Review of attachment 358139 [details] [review]: I will continue with the reviews later. Thanks for the patches. Please address my suggestions above when you can. ::: data/interface/adwaita.css @@ +144,2 @@ box.storage { background-color: #ffffff couldn't it use a known style-class such as "content-view", "view"? Have you tested with different themes and their variants?
Review of attachment 352643 [details] [review]: Pushed as a73b525d82c372357422db3d76780f7cd3c188a7.
Review of attachment 352646 [details] [review]: Pushed as 334621d9a2dd3b24cef7591d6e1b68a69aec5c23.
Attachment 358137 [details] pushed as 84a8857 - storage-analyzer: Make StorageAnalyzer class singleton Attachment 358138 [details] pushed as 12849cd - color-rectangle: Use property & constructor
Created attachment 358331 [details] [review] process: Replace getters and setter with properties Instead of use variables and getters and setters we can use properties. This change will alow easier coding for as.
Created attachment 358332 [details] [review] settings: Make Settings class singleton
Created attachment 358333 [details] [review] process-row: Move update max_usage to separate function Move update max_usage property to separate function for better understant what this code do. Swap hardcored constant with property in settings.
Review of attachment 358140 [details] [review]: thank you!
Review of attachment 358141 [details] [review]: sure!
Review of attachment 358331 [details] [review]: Ok. Let's not make this commits that change everything at once. Changes need to be smaller and graduate because it will be hard to find bugs introduced by huge commits like this one.
Review of attachment 358332 [details] [review]: ok
Review of attachment 358333 [details] [review]: ok
Review of attachment 358139 [details] [review]: This patch is huge. I will get back to it later. ::: data/interface/adwaita-dark.css @@ -132,3 @@ } -row.folders, list.folders { Drop all the changes from adwaita-dark. We are no longer shipping this file.
Comment on attachment 358332 [details] [review] settings: Make Settings class singleton Thanks! Attachment 358332 [details] pushed as 1b70dc7 - settings: Make Settings class singleton
Comment on attachment 358331 [details] [review] process: Replace getters and setter with properties Attachment 358331 [details] pushed as 1b2cad2 - process: Replace getters and setter with properties
Review of attachment 358333 [details] [review]: Almost same patch is already in master, so this not needed.
Created attachment 363161 [details] [review] storage: Refactore and rework storage Refactore and rework storage with async loading support.
(In reply to Petr Štětka from comment #54) > Created attachment 363161 [details] [review] [review] > storage: Refactore and rework storage > > Refactore and rework storage with async loading support. Dropped changes from adwaita-dark.css, and rebased to actual version.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/gnome-usage/-/issues/ Thank you for your understanding and your help.