Adding additional IOutputViewModel

I suggest that we provide model interface such as,

class IOutputViewModel
{
    void activate(QModelIndex)
    QModelIndex highlightNext()
    QModelIndex highlightPrev()
    Qlist<QMenu> contextMenu(QModelIndex)
}
and make the users double-inherit with QAbsItemModel
Reason is that

1. think about makebuilders' show next error stuff, which I
implemented.
Although the item can be activated, item cannot be selected
in GUI
because item selection is done in
QAbsItemView::setCurrentIndex().
And it's a bad idea to provides
IOutputView::setCurrentIndex() stuff
because view are hided from outside world.
By above interface, outputview knows which item to
highlight.

Also, we can consolidate many "show next item.."
stuffs. For example,
suppose that grepview, makebuilder both plugs "show
next item.."
into the menubar. It's annoying. Rather we just let only
outputview plugs
"show next item" action.

2. Context menu is eaily retrieved.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 11.06.07 04:04:28, dukju ahn wrote:
> I suggest that we provide model interface such as,
> 
> class IOutputViewModel
> {
>     void activate(QModelIndex)
>     QModelIndex highlightNext()
>     QModelIndex highlightPrev()
>     Qlist<QMenu> contextMenu(QModelIndex)
> }
> and make the users double-inherit with QAbsItemModel

So the outputview then does some dynamic_casting if he wants
to activate
or highlight right?

> 1. think about makebuilders' show next error stuff,
which I implemented.
> Although the item can be activated, item cannot be
selected in GUI
> because item selection is done in
QAbsItemView::setCurrentIndex().
> And it's a bad idea to provides
IOutputView::setCurrentIndex() stuff
> because view are hided from outside world.
> By above interface, outputview knows which item to
highlight.

I don't see how the above would help here, but maybe I just
can't
imagine enough  Would you
please provide a compilable patch for this
part at least.

> Also, we can consolidate many "show next
item.." stuffs. For example,
> suppose that grepview, makebuilder both plugs
"show next item.."
> into the menubar. It's annoying. Rather we just let
only outputview plugs
> "show next item" action.

I agree completely here.

> 2. Context menu is eaily retrieved.

That part is wrong. We already have a way to let the plugins
provide
context actions. So if we need a context menu on the
outputview (I don't
see any users yet, so there's no need to do it right now,
IMHO) we
should create a new Context subclass and go the standard way
of
retrieving the context menu.

Andreas

-- 
You have an unusual understanding of the problems of human
relationships.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/6/11, Andreas Pakulat <apakugmx.de>:
> On 11.06.07 04:04:28, dukju ahn wrote:
> > I suggest that we provide model interface such
as,
> >
> > class IOutputViewModel
> > {
> >     void activate(QModelIndex)
> >     QModelIndex highlightNext()
> >     QModelIndex highlightPrev()
> >     Qlist<QMenu> contextMenu(QModelIndex)
> > }
> > and make the users double-inherit with
QAbsItemModel
>
> So the outputview then does some dynamic_casting if he
wants to activate
> or highlight right?

Yes. right. Just think about how was the old IOutputViewItem
used.

> > 1. think about makebuilders' show next error
stuff, which I implemented.
> > Although the item can be activated, item cannot be
selected in GUI
> > because item selection is done in
QAbsItemView::setCurrentIndex().
> > And it's a bad idea to provides
IOutputView::setCurrentIndex() stuff
> > because view are hided from outside world.
> > By above interface, outputview knows which item to
highlight.
>
> I don't see how the above would help here, but maybe I
just can't
> imagine enough  Would you
please provide a compilable patch for this
> part at least.

Some skelecton code would be more understandable than full
code patch.

// user clicked menu, menu slot is executed.

StdOutputView::slotSearchNext()
{
    emit searchNext();
}

OutputViewWidget::slotSearchNext()
{
    foreach( each registered tab )
    {
        if( this tab was set default )
        {
            OutputListView view =
dynamic_cast<OutputListView*> (tab->widget());
            view->highlightNext();
        }
    }
}

class OutputListView : public QListView
{
    void highlightNext()
    {
        OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
        QModelIndex tobeHighlightedIndex =
model->highlightNext();
        setCurrentIndex( tobeHighlightedIndex );
    }

    void activate( QModelIndex index )
    {
        OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
        model->activate( index );
    }
}

class OutputViewModel : public QStandardItemModel, public
IOutputViewModel
{
    virtual QModelIndex hightlightNext() = 0;
    virtual void activate( QModelIndex index ) = 0;
}

class MakeBuilderModel : public OutputViewModel
{
    virtual QModelIndex highlightNext()
    {
        m_lastHighlight++;
        // do some calcaulation.
        return QModelIndex( m_lastHighlight );
    }

    int m_lastHighlight;
}


> > 2. Context menu is eaily retrieved.
>
> That part is wrong. We already have a way to let the
plugins provide
> context actions. So if we need a context menu on the
outputview (I don't
> see any users yet, so there's no need to do it right
now, IMHO) we
> should create a new Context subclass and go the
standard way of
> retrieving the context menu.

Yes. your approach is better.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 11.06.07 09:46:18, dukju ahn wrote:
> 2007/6/11, Andreas Pakulat <apakugmx.de>:
> > On 11.06.07 04:04:28, dukju ahn wrote:
> > > 1. think about makebuilders' show next error
stuff, which I implemented.
> > > Although the item can be activated, item
cannot be selected in GUI
> > > because item selection is done in
QAbsItemView::setCurrentIndex().
> > > And it's a bad idea to provides
IOutputView::setCurrentIndex() stuff
> > > because view are hided from outside world.
> > > By above interface, outputview knows which
item to highlight.
> >
> > I don't see how the above would help here, but
maybe I just can't
> > imagine enough  Would you
please provide a compilable patch for this
> > part at least.
> 
> Some skelecton code would be more understandable than
full code patch.

Thanks.

> // user clicked menu, menu slot is executed.
> 
> StdOutputView::slotSearchNext()
> {
>     emit searchNext();
> }
>
> OutputViewWidget::slotSearchNext()
> {
>     foreach( each registered tab )
>     {
>         if( this tab was set default )
>         {
>             OutputListView view =
dynamic_cast<OutputListView*> (tab->widget());

This is not needed, the outputwidget already has a list of
the
listviews. But thats just minor 

> class OutputListView : public QListView
> {
>     void highlightNext()
>     {
>         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
>         QModelIndex tobeHighlightedIndex =
model->highlightNext();
>         setCurrentIndex( tobeHighlightedIndex );
>     }
> 
>     void activate( QModelIndex index )
>     {
>         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
>         model->activate( index );
>     }
> }
> 
> class OutputViewModel : public QStandardItemModel,
public IOutputViewModel
> {
>     virtual QModelIndex hightlightNext() = 0;
>     virtual void activate( QModelIndex index ) = 0;
> }
> 
> class MakeBuilderModel : public OutputViewModel
> {

Uhm, for what do you need the IOutputViewModel? Just put the
two new
functions into the already existing OutputModel class and
you're done.
Then of course all plugins have to use our OutputModel as
base class and
we can change the IOutputView interface to only take
OutputModels.

There's no need for the IOutputViewModel, in fact it is
confusing,
because if a plugin only uses IOutputViewModel your
OutputListView will
break, because that one expects OutputModel.

Andreas

-- 
You have a strong desire for a home and your family
interests come first.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/6/11, Andreas Pakulat <apakugmx.de>:
> On 11.06.07 09:46:18, dukju ahn wrote:
> > 2007/6/11, Andreas Pakulat <apakugmx.de>:
> > > On 11.06.07 04:04:28, dukju ahn wrote:
> > > > 1. think about makebuilders' show next
error stuff, which I implemented.
> > > > Although the item can be activated, item
cannot be selected in GUI
> > > > because item selection is done in
QAbsItemView::setCurrentIndex().
> > > > And it's a bad idea to provides
IOutputView::setCurrentIndex() stuff
> > > > because view are hided from outside
world.
> > > > By above interface, outputview knows
which item to highlight.
> > >
> > > I don't see how the above would help here,
but maybe I just can't
> > > imagine enough  Would you
please provide a compilable patch for this
> > > part at least.
> >
> > Some skelecton code would be more understandable
than full code patch.
>
> Thanks.
>
> > // user clicked menu, menu slot is executed.
> >
> > StdOutputView::slotSearchNext()
> > {
> >     emit searchNext();
> > }
> >
> > OutputViewWidget::slotSearchNext()
> > {
> >     foreach( each registered tab )
> >     {
> >         if( this tab was set default )
> >         {
> >             OutputListView view =
dynamic_cast<OutputListView*> (tab->widget());
>
> This is not needed, the outputwidget already has a list
of the
> listviews. But thats just minor 
>
> > class OutputListView : public QListView
> > {
> >     void highlightNext()
> >     {
> >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> >         QModelIndex tobeHighlightedIndex =
model->highlightNext();
> >         setCurrentIndex( tobeHighlightedIndex );
> >     }
> >
> >     void activate( QModelIndex index )
> >     {
> >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> >         model->activate( index );
> >     }
> > }
> >
> > class OutputViewModel : public QStandardItemModel,
public IOutputViewModel
> > {
> >     virtual QModelIndex hightlightNext() = 0;
> >     virtual void activate( QModelIndex index ) =
0;
> > }
> >
> > class MakeBuilderModel : public OutputViewModel
> > {
>
> Uhm, for what do you need the IOutputViewModel?

As I said in original message, to invoke
QAbstractItemView::setCurrentIndex( tobeHighlightedIndex ).
The external user should be able to select any item by
calling setCurrentIndex()
, but the view is hidden behind ViewPlugin.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 11.06.07 11:14:12, dukju ahn wrote:
> 2007/6/11, Andreas Pakulat <apakugmx.de>:
> > On 11.06.07 09:46:18, dukju ahn wrote:
> > > class OutputListView : public QListView
> > > {
> > >     void highlightNext()
> > >     {
> > >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> > >         QModelIndex tobeHighlightedIndex =
model->highlightNext();
> > >         setCurrentIndex( tobeHighlightedIndex
);
> > >     }
> > >
> > >     void activate( QModelIndex index )
> > >     {
> > >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> > >         model->activate( index );
> > >     }
> > > }
> > >
> > > class OutputViewModel : public
QStandardItemModel, public IOutputViewModel
> > > {
> > >     virtual QModelIndex hightlightNext() =
0;
> > >     virtual void activate( QModelIndex index
) = 0;
> > > }
> > >
> > > class MakeBuilderModel : public
OutputViewModel
> > > {
> >
> > Uhm, for what do you need the IOutputViewModel?
> 
> As I said in original message, to invoke
> QAbstractItemView::setCurrentIndex(
tobeHighlightedIndex ).
> The external user should be able to select any item by
calling setCurrentIndex()
> , but the view is hidden behind ViewPlugin.

You misunderstand me  What I was
asking was: Why do you have
OutputViewModel and IOutputViewModel, but don't allow to use
the latter
one? You're casting the view's model() to OutputViewModel
in
highlightNext/Prev and activate. Thus any user of the
outputview
interface has to create an OutputViewModel subclass. Then
the
IOutputViewModel class is totally useless.

If that was just a typo, then I haven't said anything  

Andreas

-- 
You will be surprised by a loud noise.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/6/11, Andreas Pakulat <apakugmx.de>:
> On 11.06.07 11:14:12, dukju ahn wrote:
> > 2007/6/11, Andreas Pakulat <apakugmx.de>:
> > > On 11.06.07 09:46:18, dukju ahn wrote:
> > > > class OutputListView : public QListView
> > > > {
> > > >     void highlightNext()
> > > >     {
> > > >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> > > >         QModelIndex tobeHighlightedIndex
= model->highlightNext();
> > > >         setCurrentIndex(
tobeHighlightedIndex );
> > > >     }
> > > >
> > > >     void activate( QModelIndex index )
> > > >     {
> > > >         OutputViewModel *model =
dynamic_cast<OutputViewModel>(this->model());
> > > >         model->activate( index );
> > > >     }
> > > > }
> > > >
> > > > class OutputViewModel : public
QStandardItemModel, public IOutputViewModel
> > > > {
> > > >     virtual QModelIndex hightlightNext()
= 0;
> > > >     virtual void activate( QModelIndex
index ) = 0;
> > > > }
> > > >
> > > > class MakeBuilderModel : public
OutputViewModel
> > > > {
> > >
> > > Uhm, for what do you need the
IOutputViewModel?
> >
> > As I said in original message, to invoke
> > QAbstractItemView::setCurrentIndex(
tobeHighlightedIndex ).
> > The external user should be able to select any
item by calling setCurrentIndex()
> > , but the view is hidden behind ViewPlugin.
>
> You misunderstand me  What I was
asking was: Why do you have
> OutputViewModel and IOutputViewModel, but don't allow
to use the latter
> one? You're casting the view's model() to
OutputViewModel in
> highlightNext/Prev and activate. Thus any user of the
outputview
> interface has to create an OutputViewModel subclass.
Then the
> IOutputViewModel class is totally useless.
>
> If that was just a typo, then I haven't said anything


You are right. We use IOutputViewModel. We don't need
OutputViewModel.

I attatched real working code patch. I didn't commit yet
because it assumes
that there's only one view, and current tab is an active
one, the temporary
assumptions that will be addressed eventually.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

On 11.06.07 15:21:14, dukju ahn wrote:
> You are right. We use IOutputViewModel. We don't need
OutputViewModel.

Would you please put the IOutputViewModel into a separate
header and
implementation. Its not obvious that the interface that is
needed is
inside the outputmodel header. Also not all plugins may want
to use the
standard outputmodel...

> I attatched real working code patch. I didn't commit
yet because it assumes
> that there's only one view, and current tab is an
active one, the temporary
> assumptions that will be addressed eventually.

I think you can commit this (unless it doesn't compile),
after fixing
the following things (the move mentioned above). I'll add
the toolbutton
for now to make the activation thing work afterwards.

> -void OutputWidget::slotCurrentChanged( int /*index*/
)
> +void OutputWidget::selectNextItem()
>  {
> +    // TODO tab selection !!
>      QWidget* widget = currentWidget();
>      if( !widget )
>          return;
> -    if( m_widgetMap.contains( widget ) )
> +    QListView *listview =
dynamic_cast<QListView*>(widget);

Do you actually need a QListView here? If a
QAbstractItemView* is
sufficient please use that, then its easier to change from
QListView to
something different if need arises.

> +    action =
actionCollection()->addAction("next_error");
> +    action->setText("Next Error");
> +    action->setShortcut( QKeySequence(Qt::Key_F4)
);
> +    connect(action, SIGNAL(triggered(bool)), this,
SIGNAL(selectNextItem()));

Hmm, this is not good at all. Its not always an
"error", for example in
grepview, its rather result. Maybe we only have
"Next" here for now and
then see if that works.

Andreas

-- 
Beware the one behind you.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel

2007/6/11, Andreas Pakulat <apakugmx.de>:
> On 11.06.07 15:21:14, dukju ahn wrote:
> > You are right. We use IOutputViewModel. We don't
need OutputViewModel.
>
> Would you please put the IOutputViewModel into a
separate header and
> implementation. Its not obvious that the interface that
is needed is
> inside the outputmodel header. Also not all plugins may
want to use the
> standard outputmodel...
>
> > I attatched real working code patch. I didn't
commit yet because it assumes
> > that there's only one view, and current tab is an
active one, the temporary
> > assumptions that will be addressed eventually.
>
> I think you can commit this (unless it doesn't
compile), after fixing
> the following things (the move mentioned above). I'll
add the toolbutton
> for now to make the activation thing work afterwards.

Every comments from you were applied, and committed.
Thanks for reviewing.

_______________________________________________
KDevelop-devel mailing list
KDevelop-develkdevelop.org
https://barney.cs.uni-potsdam.de/mailman/listinf
o/kdevelop-devel