GNOME Bugzilla – Bug 650352
Add workspace-indicator extensions
Last modified: 2011-07-12 23:32:36 UTC
Hi: I was asked to file a bug here to include my workspace-indicator extensions in the main repository Here is the address: https://github.com/erick2red/shell-extensions
Can you please prepare a patch against git?
(In reply to comment #1) > Can you please prepare a patch against git? I do, but I think, It's better to merge my git with yours ? you could try that. and If it doesn't work, I'll do the patch. Sorry I took so long to answer but It seems, I'm not using bugzilla right, cause the mail notification didn't reach me.
Sorry for late answer. I would like to see a patch (git-rebase + git-format-patch) against current master, that I can review, before it goes in.
Created attachment 191229 [details] [review] Patch, made for adding workspace-indicator extension
Is this what you were asking for ? I don't know too well how to make patches with this. But here's a patch, let me know if you need me to do something else. Please sorry that I'm still learning the dynamics of this. If this doesn't please you. I can merge my changes on master and then send the patch, Cause now i notice my patch remove a bunch of work you made on 'configure.ac' and 'es.po' and friends.
Review of attachment 191229 [details] [review]: Yes, this is what I was expecting, thanks! ::: configure.ac @@ +24,3 @@ dnl (so basically only menus, status icons, search providers, overview tabs, message tray sources, etc.) +DEFAULT_EXTENSIONS="workspace-indicator" +ALL_EXTENSIONS="$DEFAULT_EXTENSIONS workspace-indicator" Except that of course, you don't want to remove all the other extensions. ::: extension.mk @@ +3,3 @@ # Change these to modify how installation is performed topextensiondir = $(datadir)/gnome-shell/extensions +extensionbase = @erick.red.gmail.com Of course, this should not change. ::: extensions/workspace-indicator/Makefile.am @@ +3,3 @@ +include ../../extension.mk + +@INTLTOOL_XML_NOMERGE_RULE@ You don't have xml files to read, so you don't need this. ::: extensions/workspace-indicator/extension.js @@ +9,3 @@ +const Main = imports.ui.main; + +const Gettext = imports.gettext.domain('shell-extensions'); gnome-shell-extensions is the proper domain @@ +12,3 @@ +const _ = Gettext.gettext; + +PanelMenu.SystemStatusButton.prototype.updateActor = function(_newActor){ Can you avoid adding methods to foreign classes? @@ +26,3 @@ + _init: function(){ + //debugging + Panel.mymenu = this; You don't need this, Panel exposes _statusArea with all items. @@ +28,3 @@ + Panel.mymenu = this; + + PanelMenu.SystemStatusButton.prototype._init.call(this, 'folder', 'Workspace Indicator'); Generally speaking, tooltips are frowned upon in the status area (and we may just remove support at some point). In any case, you should use _("Workspace Indicator"), to ensure translation. @@ +35,3 @@ + this.workspacesItems = []; + this._workspaceSection = new PopupMenu.PopupMenuSection(); + this.menu.addMenuItem(this._workspaceSection); You have just one PopupMenuSection. What for? Can't you put everything in the top level menu? @@ +44,3 @@ + //styling + this.menu.actor.add_style_class_name('shorter'); + this.menu.actor.remove_style_class_name('popup-menu'); Don't remove style classes, override styling in CSS (your file should have higher precedence) @@ +55,3 @@ + workspaceIndex = global.screen.get_active_workspace().index(); + } + return _('Workspace') + ' ' + ((workspaceIndex + 1).toString()); Use _("Workspace %d").format(workspaceIndex + 1), to accomodate languages that cannot use simple concatenation. (Btw, using Meta.prefs_get_workspace_name() will return the workspace name from GConf, if you want) @@ +80,3 @@ + _activate : function (index) { + let item = this.workspacesItems[index]; + if(!item) return; Do you need this check? Can't you just check that index is between 0 and n_workspaces()? @@ +83,3 @@ + let metaWorkspace = global.screen.get_workspace_by_index(index); + metaWorkspace.activate(true); + this._updateIndicator(); The indicator should be updated automatically by 'workspace-switched' signal anyway.
(In reply to comment #6) > Review of attachment 191229 [details] [review]: > > Yes, this is what I was expecting, thanks! > > ::: configure.ac > @@ +24,3 @@ > dnl (so basically only menus, status icons, search providers, overview tabs, > message tray sources, etc.) > +DEFAULT_EXTENSIONS="workspace-indicator" > +ALL_EXTENSIONS="$DEFAULT_EXTENSIONS workspace-indicator" > > Except that of course, you don't want to remove all the other extensions. That was what I meant with no sure if this is what you need, Yeah I don't want to remove the other exts. > ::: extensions/workspace-indicator/Makefile.am > @@ +3,3 @@ > +include ../../extension.mk > + > +@INTLTOOL_XML_NOMERGE_RULE@ > > You don't have xml files to read, so you don't need this. I'll remove it right away > ::: extensions/workspace-indicator/extension.js > @@ +9,3 @@ > +const Main = imports.ui.main; > + > +const Gettext = imports.gettext.domain('shell-extensions'); > > gnome-shell-extensions is the proper domain Yeap, I knew I just wasn't to sure about it > @@ +12,3 @@ > +const _ = Gettext.gettext; > + > +PanelMenu.SystemStatusButton.prototype.updateActor = function(_newActor){ > > Can you avoid adding methods to foreign classes? I do need to replace the actor for the number, I'll see it done. > @@ +26,3 @@ > + _init: function(){ > + //debugging > + Panel.mymenu = this; > > You don't need this, Panel exposes _statusArea with all items. Sure ? OK > @@ +28,3 @@ > + Panel.mymenu = this; > + > + PanelMenu.SystemStatusButton.prototype._init.call(this, 'folder', > 'Workspace Indicator'); > > Generally speaking, tooltips are frowned upon in the status area (and we may > just remove support at some point). > In any case, you should use _("Workspace Indicator"), to ensure translation. Let's then removed the tip > @@ +35,3 @@ > + this.workspacesItems = []; > + this._workspaceSection = new PopupMenu.PopupMenuSection(); > + this.menu.addMenuItem(this._workspaceSection); > > You have just one PopupMenuSection. What for? Can't you put everything in the > top level menu? Newbie, this was my first attempt of an extension. > @@ +44,3 @@ > + //styling > + this.menu.actor.add_style_class_name('shorter'); > + this.menu.actor.remove_style_class_name('popup-menu'); > > Don't remove style classes, override styling in CSS (your file should have > higher precedence) This was caused by that bug is now solved about the user-style-sheet not proper loading, or something like that, nasty bug. > @@ +55,3 @@ > + workspaceIndex = global.screen.get_active_workspace().index(); > + } > + return _('Workspace') + ' ' + ((workspaceIndex + 1).toString()); > > Use _("Workspace %d").format(workspaceIndex + 1), to accomodate languages that > cannot use simple concatenation. > (Btw, using Meta.prefs_get_workspace_name() will return the workspace name from > GConf, if you want) Better thought > @@ +80,3 @@ > + _activate : function (index) { > + let item = this.workspacesItems[index]; > + if(!item) return; > > Do you need this check? Can't you just check that index is between 0 and > n_workspaces()? Course I can > @@ +83,3 @@ > + let metaWorkspace = global.screen.get_workspace_by_index(index); > + metaWorkspace.activate(true); > + this._updateIndicator(); > > The indicator should be updated automatically by 'workspace-switched' signal > anyway. You mean not calling "this._updateIndicator()" twice ? OK Now I don't know if you mean I should fix this before inclusion. Anyway I did it and the patch is here.
Created attachment 191737 [details] [review] This is a fix patch including the old changes, and the new ones overwriting the stuff that were made wrong.
Comment on attachment 191737 [details] [review] This is a fix patch including the old changes, and the new ones overwriting the stuff that were made wrong. Rebased, squashed, fixed, committed!